-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add set_baud() method to UARTSerial(). #4615
Conversation
@kjbracey-arm, @hasnainvirk: thought I'd propose this, though am happy for you to propose a different solution if you prefer. |
@kjbracey-arm, @hasnainvirk bump |
retest uvisor |
* | ||
* @param baud The baud rate | ||
*/ | ||
void set_baud(int baud); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use the baud method from SerialBase instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kjbracey-arm: Kevin, are you OK with the convention of simply calling in the SerialBase
method rather than having the intermediate that adds 'set_'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The question is why SerialBase is inherited privately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A philosophical/religious question for @kjbracey-arm I think...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's private because if any user messes with the underlying port while we're using it, it could break us - at least if they go anywhere near the core data functions.
This is a driver that uses the underlying serial port itself - I don't want to expose it to the users of this class so they can freely touch it. Making it a public base class would expose more than I want.
In this particular case "baud" could directly pass through, as we don't care (beyond maybe it being a good idea to drain the output buffer?).
But "set_flow_control" we would potentially want to implement ourself rather than pass through to provide XON/XOFF or manual HW control.
There's existing inconsistency on the "set_" prefix ("baud" vs "set_flow_control"), so it seems reasonable to unify on the "set_" to me.
General note - we are currently lacking is a "serial control" abstract interface for these things (baud, format, flow control, hup) - each of the buffered serial, the raw serial, and the serial mux should all be providing that as a common interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's existing inconsistency on the "set_" prefix ("baud" vs "set_flow_control"), so it seems reasonable to unify on the "set_" to me.
General note - we are currently lacking is a "serial control" abstract interface for these things (baud, format, flow control, hup) - each of the buffered serial, the raw serial, and the serial mux should all be providing that as a common interface.
+1
Thanks for the explanation @kjbracey-arm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant to request changes, my bad.
I was sorta following the convention that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Convention has been to prefix the method name by set_ , I am happy with it.
/morph test |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
OutputAll builds and test passed! |
Description
Add a
set_baud()
method toUARTSerial()
, simply handing it on toSerialBase:baud()
. See #4609 for reasons why this is necessary.With this in place the
CellularInterface
can instantiate the file handle it requires withUARTSerial()
running at a baud rate of 115200 (which all modems can be relied upon to auto-baud at). Then, once the modem is up and running, it can send the modem anAT+IPR
command giving a higher baud rate that the modem supports but would not be able to auto-baud at (e.g. 460800). This way we can get nice fast connections.This has been tested on the u-blox C030 board with a Sara-U201 modem, eventually running at 460800 bits/s.