Templatized serial classes #11982

ejtagle · 2018-10-02T06:22:29Z

This pull request implements templatized Serial classes, allowing to instance them more than once, one for each required hardware serial port, and keeping the static nature of functions (no virtual overhead)

It is implemented for both AVR and Due platforms.

Any suggestion, question, improvement, just ask!

I have tested them lightly, and they work. The logic is exactly the same as before

thinkyhead · 2018-10-02T11:37:22Z

In spite of working for you, the changes weren't passing Travis CI. One issue was that you can't use ENABLED for undefined symbols outside of preprocessor directives.

The other issue is that the rx_buffer and tx_buffer initializers were failing, citing a bad initializer for "unsigned char[0]", which might be due to the RX_SIZE/TX_SIZE not getting into the struct declarations.

    unsigned char buffer[RX_SIZE]; // <--- is RX_SIZE picked up as 0 here?

Does the struct need a template<...> prefix to get RX_SIZE? What do you suppose is going on?

My temporary workaround was just to drop the = { { 0 }, 0, 0 } initializer on the buffer structs. I doubt this causes the buffer to inherit the correct length, if that is indeed the issue. But it suppresses the compile error. I definitely recommend looking deeper into the issue.

thinkyhead · 2018-10-02T11:45:58Z

I've also just added a commit to make the code more readable (and maintainable) by defining macros for the long template signatures. So in future they will only need to be changed in one place instead of several.

dgrat · 2018-10-02T15:35:59Z

Is it possible to define a struct and pass that as template parameter?

#if SERIAL_PORT >= 0
  #define TEMPLATE_SIG int portNr, int RX_SIZE, int TX_SIZE, bool USE_XONOFF, bool USE_EMERGENCYPARSER, bool STATS_DROPPED_RX, bool STATS_RX_OVERRUNS, bool STATS_RX_FRAMING_ERRORS, bool STATS_MAX_RX_QUEUED
  #define TEMPLATE_ARG     portNr,     RX_SIZE,     TX_SIZE,      USE_XONOFF,      USE_EMERGENCYPARSER,      STATS_DROPPED_RX,      STATS_RX_OVERRUNS,      STATS_RX_FRAMING_ERRORS,      STATS_MAX_RX_QUEUED
#endif

Additionally, one could use a typedef by "using" to simplify these things:

template<TEMPLATE_SIG> typename MarlinSerial<TEMPLATE_ARG>
// ...

Finally, I do not like these specializations

template<> struct MarlinSerialPortInfo<1> {
  static constexpr unsigned int BASE = 0x40098000U; // USART0
  static constexpr IRQn_Type IRQ = USART0_IRQn;
  static constexpr int IRQ_ID = ID_USART0;
};

All the values can be directly calculated by the template parameter..
E.g. BASE is always 16384 off, etc. So why to define these specializations at all?

I like templates, but I still have the impression that these commits make the code longer and more complicated. Therefore, I would suggest to simplify it a bit more.

ejtagle · 2018-10-02T16:25:15Z

In spite of working for you, the changes weren't passing Travis CI. One issue was that you can't use ENABLED for undefined symbols outside of preprocessor directives.

The other issue is that the rx_buffer and tx_buffer initializers were failing, citing a bad initializer for "unsigned char[0]", which might be due to the RX_SIZE/TX_SIZE not getting into the struct declarations.
    unsigned char buffer[RX_SIZE]; // <--- is RX_SIZE picked up as 0 here?
Does the struct need a template<...> prefix to get RX_SIZE? What do you suppose is going on?

My temporary workaround was just to drop the = { { 0 }, 0, 0 } initializer on the buffer structs. I doubt this causes the buffer to inherit the correct length, if that is indeed the issue. But it suppresses the compile error. I definitely recommend looking deeper into the issue.

Well, the main problem seems to be that when using TX_SIZE = 0 (the default Marlin configuration), the structure gets defined as

      struct ring_buffer_t {
      unsigned char buffer[0];
      volatile uint8_t head, tail;
    };

And then the initializer does not work, as it is

{ { 0 }, 0, 0 }

And that would assign a 0 to an empty array. According to c/c++ standard, if i initialize any member, then all the other missing members of a struct will be implicitly initialized to 0.

Maybe the best approach would be to reorder those structs:

      struct ring_buffer_t {
      volatile uint8_t head, tail;
      unsigned char buffer[TX_SIZE];
    };

and then initialize them by

{ 0 }

As the struct is declared inside the template, it does require most of that template stuff .. And you are right: I don´t like how long it is...

ejtagle · 2018-10-02T16:27:00Z

I've also just added a commit to make the code more readable (and maintainable) by defining macros for the long template signatures. So in future they will only need to be changed in one place instead of several.

I do agree with this. I wasn´t sure if it would be acceptable to do it. That is why i didn´t 👍

ejtagle · 2018-10-02T16:30:32Z

@dgrat : I haven´t tried the idea of passing an struct as template parameter. But a configuration type could work, you are right. Maybe that could be a better practice here...

ejtagle · 2018-10-02T16:35:59Z

Is it possible to define a struct and pass that as template parameter?
#if SERIAL_PORT >= 0
  #define TEMPLATE_SIG int portNr, int RX_SIZE, int TX_SIZE, bool USE_XONOFF, bool USE_EMERGENCYPARSER, bool STATS_DROPPED_RX, bool STATS_RX_OVERRUNS, bool STATS_RX_FRAMING_ERRORS, bool STATS_MAX_RX_QUEUED
  #define TEMPLATE_ARG     portNr,     RX_SIZE,     TX_SIZE,      USE_XONOFF,      USE_EMERGENCYPARSER,      STATS_DROPPED_RX,      STATS_RX_OVERRUNS,      STATS_RX_FRAMING_ERRORS,      STATS_MAX_RX_QUEUED
#endif
Additionally, one could use a typedef by "using" to simplify these things:
template<TEMPLATE_SIG> typename MarlinSerial<TEMPLATE_ARG>
// ...
Finally, I do not like these specializations
template<> struct MarlinSerialPortInfo<1> {
  static constexpr unsigned int BASE = 0x40098000U; // USART0
  static constexpr IRQn_Type IRQ = USART0_IRQn;
  static constexpr int IRQ_ID = ID_USART0;
};
All the values can be directly calculated by the template parameter..
E.g. BASE is always 16384 off, etc. So why to define these specializations at all?

I like templates, but I still have the impression that these commits make the code longer and more complicated. Therefore, I would suggest to simplify it a bit more.

The idea of those specializations is to reuse the CMSIS defined symbols, instead of hardcoding those values. The main problem is that i wanted constexprs (otherwise, SRAM is wasted to store non-changing values!! and code size increases)
But constexpr pointers can`t be initialized to absolute values (as the ones defined in CMSIS headers), that is why i ended using those structs with specializations...

I always felt that the using keyword was, when used to hide long lines, was more a hack... I will try the configuration struct idea...

ejtagle · 2018-10-02T19:09:06Z

@thinkyhead , @dgrat ... Changed to a struct as template parameter to configure the class...
You were right, it is much cleaner, and this time I like the resulting simplification and reduction of source code

ejtagle · 2018-10-03T01:23:24Z

@thinkyhead : Unfortunately, ring_buffer_size_t will never work outside the MarlinSerial class, as this type definition depends on the template parameters

But, do not worry about how much space the struct MarlinSerialCfg will take. This struct is never instantiated, so it does not take neither SRAM nor FLASH space! 🥇

We only use the type, but never instantiate it... advantages of the static constexpr qualifier, that allows us to define a constant value and access it without memory usage! 👍 - So i suggest to keep unsigned int as the type of the RX_SIZE/TX_SIZE on that configuration struct - It is the easiest way to do it

On the MarlinSerial class, the type changes based on the maximum size of the buffer: > 256 will use a uint16_t, less than that will use a uint8_t

thinkyhead · 2018-10-03T02:22:05Z

In that case, reverted!

thinkyhead · 2018-10-03T02:25:19Z

So… How far does this go to eliminating the need for HardwareSerial and SoftwareSerial classes? Is it possible we could get rid of Marlin's dependency on the Stream classes altogether? And, apart from AVR / DUE, does it really matter?

From here it would be good to have some documentation on how Marlin handles serial I/O and how to set up new serial devices to use the best available option.

ejtagle · 2018-10-03T02:35:29Z

@thinkyhead : HardwareSerial dependencies are gone (at least, for AVR and DUE).
SoftwareSeria is a can of worms, trust me: For a personal project, i ran into several unsolvable issues with SoftwareSerial. The main problem is that it not only is half duplex, but also if instantiating more than one SoftwareSerial, only one of the instances can be receiving at any time... And also, SoftwareSerial timing was horrible (to say the least) when going above 9600 bauds. I had ton of issues with that library. I even used a logic analyzer to deduce what was going on...
Eventually i had to rewrite it from scratch with some assembly routines. And the maximum reliable speed was 9600 bauds on AVR, due to the "one ISR per bit" nature of this implementation.
My suggestion: Avoid SoftwareSerial as the plague: Unreliable to say the least - And the problem is unsolvable, as to properly implement a Software UART, you need: 3x the baudrate of ISRs per second, or to have a hardware compare (time compare) pin (in such case 1x the baudrate in ISRs, or you risk losing synchronization) -- SoftwareSerial uses busy waiting without disabling interrupts to receive, so it is essentially a disaster on Marlin, that preempts the delay with several other interrupts...
Transmission is "easier", but requires precise timing, and 1x baudrate ISRs per second (or to completely disable interrupts while sending data).
All those requirements are completely incompatible with Marlin Stepper ISR, so, i´d suggest to avoid using SoftwareSerial. It will never be a reliable substitution for a HardwareSerial interface.

BTW: This PR objective was to allow more than 1 HardwareUart as would be required for MMU or anything that requires more than one hardware UART to be active at the same time

thinkyhead · 2018-10-03T02:58:58Z

Anything left to do here before we release this into the wild?

ejtagle · 2018-10-03T03:06:31Z

@thinkyhead : Basically it is working... As said before, just waiting for feedback on code styling ;)
(but i am more than happy with the last version)

ModMike · 2018-10-10T17:31:17Z

I am looking to enable a Creatbot Touch Panel (same as Wanhao and MonoPrice screen) that is connected to pins 15 RX3 & 14 TX3. How do I configure or define software serial pins for a 2560?

ejtagle · 2018-10-10T18:06:00Z

If that panel emits standard Gcode to control Marlin, you just need to #define SERIAL_PORT_2 3
And make sure nothing else is using those pins...

ModMike · 2018-10-10T18:18:35Z

That is the "assumption". I tried 3 and no luck.

Just to confirm: Where are the software serial port pins defined? In the boards pins file?

ejtagle · 2018-10-10T18:30:54Z

There is no such definition. The serial port pins are fixed by the Atmega2560 processor. They can´t be moved or remapped to other pins!
You don´t need to define those pins. The only thing you need to check is if those pins are being used for something else, either because they are assigned to a different function by Marlin (they can also be used as standard IO

ModMike · 2018-10-10T18:33:29Z

Thanks, will run pins_debug to be sure. Could also be that screen looks for a response and is not getting the right one so refuses to start. I did try different baud rates but originally was 250000.

ModMike · 2018-10-10T20:15:14Z

I enabled PINS_DEBUGGING, commented out serial port 2, and did M43. As hoped and expected I got this:

PIN:  14   Port: J1        <unused/unknown>                       Input  = 1
PIN:  15   Port: J0        <unused/unknown>                       Input  = 1

I then uncommented serial port_2 3, did an M43 and got this:

PIN:  14   Port: J1        <unused/unknown>                       Input  = 1
PIN:  15   Port: J0        <unused/unknown>                       Input  = 1

So pins still not assigned. Seems like the pins are not defined somewhere or that 3 is the wrong port. Any ideas?

ejtagle · 2018-10-10T20:28:48Z

The pins are not reflected as assigned. That is good news. The only pins that will reflect as assigned are IO pins, but UART are not... as they are being used for UART, not for general purpose IO

ModMike · 2018-10-10T20:35:10Z

So you are saying that they are actually assigned even though they are not showing up as such?

When using TMC software serial you need to define them in the boards file, why would you not do the same here? We are talking about software serial right?

ejtagle · 2018-10-10T20:44:21Z

No, this is hardware serial.
SoftwareSerial is a very different (and problematic) beast!

ModMike · 2018-10-10T20:54:46Z

Ok let me start from the beginning. I am trying to use a touch panel that uses g-code (works in a manufacturers custom version of marlin). It is connected to pins 14 and 15.

Should I be using hardware serial or software serial? If so, do I need to assign those pins anywhere?

ejtagle · 2018-10-10T21:19:00Z

If it is connected to pins 14 and 15 of the atmega processor, according to its datasheet, there is no hardware UART there. Pin 14 says (XCK2/PH2), and Pin 15 says (OC4A/PH3).
You would need software serial, that configuration is unsupported, and probably unstable.

ModMike · 2018-10-10T21:22:43Z

Yes! Now I know where to start. I know people don't like it but it is being used for TMC control. Can you point me in the right direction? Where do I define the software serial pins? What is the define syntax? There is an incredible lack of documentation on this. A few people got it working but are never clear as to how.

Sorry to sound like a tiresome idiot that I am. I have been working on this for 3 weeks and need only this and the power button to finish.

AnHardt · 2018-10-10T21:43:21Z

Arduino Mega | AVR Prozessor 2650 | function
D14          | 64                 | TX3
D15          | 63                 | RX3

There is a hardware serial!

Most common error with serials is connecting RX to RX and TX to TX instead of crossing them RX to TX and TX to RX.

p3p · 2018-10-10T21:44:40Z

According to Arduino those pins are hardware serial on the 2560, https://www.arduino.cc/reference/en/language/functions/communication/serial/

(as previously stated by AnHardt ^^)

ModMike · 2018-10-10T21:57:40Z

Ok I am getting whiplash. So hardware serial it is? If understand correctly, it is baked in so all I need is:

#define SERIAL_PORT_2 3

Any other considerations?

Thank you everyone!

ejtagle · 2018-10-10T22:00:51Z

Ok I am getting whiplash. So hardware serial it is? If understand correctly, it is baked in so all I need is:

#define SERIAL_PORT_2 3

Any other considerations?

Thank you everyone!

That should be all that is needed.

paukstelis · 2018-10-11T19:40:58Z

Am I correct in thinking that this implementation pushes everything into standard isr, so it is really just for reading gcode from another device? To do general serial communication with another MCU for other purposes you are pretty much limited to HardwareSerial?

ejtagle · 2018-10-11T21:16:23Z

You are wrong. This Serial class can be used for any purpose besides reading gcode.

paukstelis · 2018-10-11T22:33:32Z

You are wrong. This Serial class can be used for any purpose besides reading gcode.

Cool. This is referenced with MYSERIAL1 for general purpose io after it has been defined?

ejtagle · 2018-10-11T23:23:01Z

No, it depends. If you define SERIAL_PORT_2 to any value, then you will get a MYSERIAL1, but it will be used for Gcode.

If you need to use a serial port for something else, then you must instantiate the template yourself with your own name, so you will get exclusive access to the serial port you need

Look at the bottom of MarlinSerial.h and at the bottom of MarlinSerial.cpp for an example of the required things for it to work

paukstelis · 2018-10-12T01:47:28Z

Great. Thanks for the info. I'll see if I can figure it out.

ejtagle added 2 commits October 2, 2018 03:15

AVR templatized MarlinSerial class, allowing multiple instances

ad934f5

DUE templatized MarlinSerial class, allowing multiple instances

96627bf

This was referenced Oct 2, 2018

Expand serial support in DUE HAL.h #11947

Closed

[PR] New config option USE_UART to enable an additional UART #11880

Closed

thinkyhead added 2 commits October 2, 2018 06:13

Fix MarlinSerial instantiation, etc.

d7ebe7e

"too many initializers" workaround

bb36e9f

Make template code more readable

dcf461b

thinkyhead added PR: Improvement C: Serial Comms labels Oct 2, 2018

Converted to use a struct as configurator for template

0e1199b

Tweaky spacing changes

4b28a22

thinkyhead merged commit f6f2246 into MarlinFirmware:bugfix-2.0.x Oct 3, 2018

ejtagle deleted the templatized-serial branch October 3, 2018 03:17

thinkyhead mentioned this pull request Jul 31, 2021

[BUG] warning: "SERIAL_TX_BUFFER_SIZE" redefined #22466

Closed

Templatized serial classes #11982

Templatized serial classes #11982

Conversation

ejtagle commented Oct 2, 2018

thinkyhead commented Oct 2, 2018 • edited Loading

thinkyhead commented Oct 2, 2018

dgrat commented Oct 2, 2018 • edited Loading

ejtagle commented Oct 2, 2018

ejtagle commented Oct 2, 2018

ejtagle commented Oct 2, 2018

ejtagle commented Oct 2, 2018

ejtagle commented Oct 2, 2018

ejtagle commented Oct 3, 2018 • edited Loading

thinkyhead commented Oct 3, 2018

thinkyhead commented Oct 3, 2018 • edited Loading

ejtagle commented Oct 3, 2018 • edited Loading

thinkyhead commented Oct 3, 2018

ejtagle commented Oct 3, 2018

ModMike commented Oct 10, 2018

ejtagle commented Oct 10, 2018

ModMike commented Oct 10, 2018

ejtagle commented Oct 10, 2018

ModMike commented Oct 10, 2018

ModMike commented Oct 10, 2018 • edited Loading

ejtagle commented Oct 10, 2018

ModMike commented Oct 10, 2018 • edited Loading

ejtagle commented Oct 10, 2018

ModMike commented Oct 10, 2018

ejtagle commented Oct 10, 2018

ModMike commented Oct 10, 2018

AnHardt commented Oct 10, 2018

p3p commented Oct 10, 2018 • edited Loading

ModMike commented Oct 10, 2018 • edited Loading

ejtagle commented Oct 10, 2018 • edited by thinkyhead Loading

paukstelis commented Oct 11, 2018

ejtagle commented Oct 11, 2018

paukstelis commented Oct 11, 2018

ejtagle commented Oct 11, 2018

paukstelis commented Oct 12, 2018

thinkyhead commented Oct 2, 2018 •

edited

Loading

dgrat commented Oct 2, 2018 •

edited

Loading

ejtagle commented Oct 3, 2018 •

edited

Loading

thinkyhead commented Oct 3, 2018 •

edited

Loading

ejtagle commented Oct 3, 2018 •

edited

Loading

ModMike commented Oct 10, 2018 •

edited

Loading

ModMike commented Oct 10, 2018 •

edited

Loading

p3p commented Oct 10, 2018 •

edited

Loading

ModMike commented Oct 10, 2018 •

edited

Loading

ejtagle commented Oct 10, 2018 •

edited by thinkyhead

Loading