Major restructure of the UART support functionality

[MartinMueller2003]

Moved uart support to its own class and made the API very similar to the new RMT API.
All protocols are now supported on all ports

[henrygab]

Overall, this is great work creating a maintainable architecture of abstraction. Logical, concise, and overall love it.

I've a few changes that I'd suggest, although any of them could occur in a later PR. See individual comments.

[henrygab]

minor nit ... can you adjust from first 2 bits ignored to most significant 2 bits ignored?
after all, the "first" bits send using UART protocol are always the least signficant bits,
whereas here, it's the two most significant bits that are ignored ... so it's would increase
clarity.

[MartinMueller2003]

Good comments. I will incorporate into next pr

[MartinMueller2003]

Done

[henrygab]

it would be great to keep the old comments for these magic values, to help folks reading it to correlate the values to the actual bitstream output. The mapping seems fairly straightforward for this one:

    {0b11111100, ... }, // (0) 0011 1111 (11)
    {0b11000000, ... }, // (0) 0000 0011 (11)

[MartinMueller2003]

Done

[henrygab]

minor nit ... can you adjust from first 2 bits ignored to most significant 2 bits ignored?
after all, the "first" bits send using UART protocol are always the least signficant bits,
whereas here, it's the two most significant bits that are ignored ... so it's would increase
clarity.

[henrygab]

Consider more descriptive names, such as:

    NoTranslation = 0,
    OneBitPerUartFrame,
    TwoBitsPerUartFrame,

[MartinMueller2003]

This would overload the term frame which in my case is used for the pixel / data frame starting at channel zero and going through the last channel data. Very few people even understand that the UART level also has the concept of a frame. I am going to keep the current names.

[henrygab]

OK. Thanks for considering my feedback. I respect your choice.

[henrygab]

Would be great to have the same UART encoding of the bits:

{ 0b11111100, ... }, // (1) 1100 0000 (0)
{ 0b11000000, ... }, // (1) 1111 1100 (0)

[MartinMueller2003]

Done

[henrygab]

Consider making this at least an inline function (taking as input the OutputUartConfig).

With the right GCC attributes, it will be treated identically to the #define, except you'll get scope and type safety (and all the other benefits of avoiding the preprocessor).

For example, it might look something close to the following:

inline bool WeNeedAtimer(OutputUartConfig_t& config) __attribute__ (( gnu_inline, const )) {
#if defined(ARDUINO_ARCH_ESP32)
    return false;
#else
    return (config.NumBreakBitsAfterIntensityData || config.NumExtendedStartBits);
#endif
}

[MartinMueller2003]

Pure Opinion: For some of us, the preprocessor is our friend. This latest disfavor with the preprocessor is largely due to the abuse of the preprocessor abilities to mimic template like functionality at a time that the C++ pushers wanted people to use the new functionality.

Macro has been converted.

[MartinMueller2003]

I have just converted this to a function in the .hpp file. You will notice that I avoid creating functions in the include files because (while we are not using it) it is my opinion that every function needs to be unit tested and functions defined in headers are notoriously difficult to test. I convinced myself that one line actions (aka macro) are an acceptable exception but a full function definition still feels like it needs a unit test and should be defined in a cpp file. I like the C# model in which header files are a thing of the past and this quandary goes away.

[henrygab]

Here's another example where this psuedo-function macro (which presumes that there is a local variable of a given name) should likely become an gnu_inline function.

The GCC attributes of interest include:

• gnu_inline -- "In no case is the function compiled as a standalone function, not even if you take its address explicitly" per the docs.
• const (more strict than even pure attribute)

For example, it might look something close to the following:

inline void ClearUartInterrupts(OutputUartConfig_t& config) __attribute__(( gnu_inline, const )) {
    WRITE_PERI_REG(UART_INT_CLR(config.UartId), UART_INTR_MASK);
}
inline void DisableUartInterrupts(OutputUartConfig_t& config) __attribute__(( gnu_inline, const )) {
    CLEAR_PERI_REG_MASK(UART_INT_ENA(config.UartId), UART_INTR_MASK);
}

[MartinMueller2003]

Those functions need to be in IRAM which the macro nicely solved. At any rate, I converted them to IRAM inline functions. However, this is pretty much a waste of time and does not enhance safety or maintainability.

[henrygab]

I recognize you as an expert coder and your strong architectural skills.

I did not intend to offend; I have tried to only comment on items where maintainability or clarity would be increased, and worked hard to meet your apparent goals in those comments.

For example, I'm confused by the leading statement about IRAM... If using gnu_inline and const attributes as I suggested, then IRAM_ATTR wouldn't apply, because the function is never instantiated (can't even get a function pointer to it) ... it's forced to act substantially identically to a function-like macro. If those attributes are not applied, then of course the IRAM_ATTR would be needed. I noticed you chose not to use the attributes.
Can you help me understand the choice not to use them, as they would have provided the behavior you seemed to be looking for?

Can you help me understand your statement, "does not enhance safety or maintainability".

• Inline function has type safety for its parameters, Macro has no such type safety
• The macro was only intended for use within the class / in combination with the class object
• The macro was in common header file, and presumed a specific local variable name
  Wouldn't these attributes show greater safety and maintainability from an inline function?

[MartinMueller2003]

• Inline function has type safety for its parameters, Macro has no such type safety
  Actually, other than the enqueue macro, the macros do not have parameters. They count on well known data elements within a single class.
• The macro was only intended for use within the class / in combination with the class object
  Correct. They are very focused and therefore forced to stay very simple.
• The macro was in common header file, and presumed a specific local variable name
  Correct. They are very focused and therefore forced to stay very simple.
• Wouldn't these attributes show greater safety and maintainability from an inline function?
  Since the functionality in a macro is kept VERY simple, converting to inline actually provides very little benefit. Where more complex actions and potentially decisions are made, I agree a function is the better choice. Where an often repeated one line operation is performed where no decisions are made and no response is expected, Where no parameters are passed in, a macro is fine.

RE: IRAM_ATTR on inline. I was getting compiler issues and did not want to spend the time to resolve them. I will look at this again when I have more time.

[henrygab]

I think we are much closer aligned than it may have seemed.

Since this was intended only for use within that one class, would you have considered moving the macro to the implementation (.cpp) file, rather than the header (.h/.hpp) file?

If so, although I would have written it as an inline function, I likely would not have mentioned it, as its scope would be limited to that single file. (Just another alternative, to support the preference for macros.)

P.S. - the functions may need to be only defined in the .cpp file, as otherwise the registers are not defined. Here's an example that compiles, when placed in the implementation file:

static inline __attribute__(( always_inline, gnu_inline ))
void IRAM_ATTR EnableUartInterrupts(c_OutputUart::OutputUartConfig_t& config) {
    SET_PERI_REG_MASK  (UART_INT_ENA(config.UartId), UART_TXFIFO_EMPTY_INT_ENA);
}
static inline __attribute__(( always_inline, gnu_inline ))
void IRAM_ATTR DisableUartInterrupts(c_OutputUart::OutputUartConfig_t& config)  {
    CLEAR_PERI_REG_MASK(UART_INT_ENA(config.UartId), UART_TXFIFO_EMPTY_INT_ENA);
}
static inline __attribute__(( always_inline, gnu_inline ))
void IRAM_ATTR ClearUartInterrupts(c_OutputUart::OutputUartConfig_t& config)  {
    WRITE_PERI_REG     (UART_INT_CLR(config.UartId), UART_INTR_MASK);
}

[MartinMueller2003]

If they are coded as members of the uart class then I am ok with it. I am not ok with passing in those parameters.