Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 7 commits into from

Conversation

revilor
Copy link
Contributor

@revilor revilor commented Sep 20, 2018

In preparation for MMU2 support I added the option to activate another UART which will be required to send commands to the MMU unit.

For the start I added MarlinUART to the AVR HAL, which is just a wrapper for Arduino Serial. I'm not 100% sure this is the way to go, because there is also MarlinSerial which could possibly be adapted for that purpose. But I didn't want to mess with host communication so I did not touch MarlinSerial.

Please have a look if the way I started is the way to go. I'm open to any suggestions. Thankx.

@thinkyhead
Copy link
Member

I guess we already have a facility to use multiple serial connections, but it seems to be tailored for sending to and receiving from multiple hosts. See the get_serial_commands command to see how it's being used currently.

And, as mentioned elsewhere, the MALYAN_LCD just uses the HAL-provided Serial1.

@ejtagle
Copy link
Contributor

ejtagle commented Sep 21, 2018

The main problem with the AVR MarlinSerial class is that all methods are static.
If we want multiple instances (one for each available serial port), the static must go away.
Doing that increases code size, but would allow to use our class for all serial ports
There is nothing bad (besides code duplication) if using both Arduino provided and our serial implementation, except for the fact Arduino provided serial ports are not interrupt enabled, so they block execution while sending and require permanent polling while receiving data...

@ejtagle
Copy link
Contributor

ejtagle commented Sep 21, 2018

@thinkyhead HAL provided Serial1 ? .. Think twice ... ;) -- Not on AVR. What HAL provides is a define that redirects to the original Arduino serial class

@thinkyhead thinkyhead force-pushed the bugfix-2.0.x branch 2 times, most recently from 9d867f9 to 849dea9 Compare September 24, 2018 03:23
@thinkyhead
Copy link
Member

thinkyhead commented Sep 30, 2018

What HAL provides is a define that redirects to the original Arduino serial class

Right. Maybe a better way to put it is, the HALs make allowances when possible.

@thinkyhead
Copy link
Member

The code size explodes quite a bit if we were to make the serial class non-static. For this reason it seems prudent to keep the static class and use it when only a single port is configured for use.

@revilor
Copy link
Contributor Author

revilor commented Oct 1, 2018

Using Arduino Serial adds a huge footprint to progmem and sram. Therefore I cloned MarlinSerial to MarlinUART and stripped it down to the core functionality. This way MarlinSerial remains static and can deal with host communication.

And in case an additional serial interface is required for internal communication (like with MMU2), MarlinUART is enabled using the USE_UART option.

@ejtagle
Copy link
Contributor

ejtagle commented Oct 2, 2018

@revilor : I have a better idea: templatize the MarlinSerial class based on selected port and options, so it keeps its static attribute, but allows multiple instances.

This can avoid the huge code duplication ...

@revilor
Copy link
Contributor Author

revilor commented Oct 2, 2018

@ejtagle Sounds legit. My C++ knowledge is from before templates, but I'll give it a try. Never too old to learn new stuff. Thankx.

@ejtagle
Copy link
Contributor

ejtagle commented Oct 2, 2018

@revilor : I already have a prototype. Probably will require some review (even if it IS working!) due to cosmetics and style... ;)

@thinkyhead
Copy link
Member

@ejtagle — Looking forward to polishing it up and giving it some tests!

@ejtagle
Copy link
Contributor

ejtagle commented Oct 2, 2018

I am at it right now... I am sure it will be "interesting"... ;)

@ejtagle
Copy link
Contributor

ejtagle commented Oct 2, 2018

@thinkyhead , @revilor PR #11982 is the implementation of the AVR and Due templatized Serial port class that allows multiple concurrent instances without the virtual function overhead ...

To instantiate an extra serial port, look at the bottom of both MarlinSerial.cpp / MarlinSerial.h. In the case of AVR, you need to declare the ISR redirectors for the extra serial port, exactly as the one that is declared, but for the other port(s)

@thinkyhead
Copy link
Member

@ejtagle — Now that the templatized serial port is merged, what are the implications for this PR?

@ejtagle
Copy link
Contributor

ejtagle commented Oct 3, 2018

@thinkyhead : After PR #11982 , this PR is completely unneeded. If @revilor needs to use an extra UART, he just needs to instantiate the template a 3rd time (at the bottom of MarlinSerial.h and MarlinSerial.cpp) and that is all ;)

@thinkyhead thinkyhead closed this Oct 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants