Serial design draft #11634

evedon · 2019-10-04T13:00:48Z

Description

This is the design proposal for cleaning-up the serial class hierarchy.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[X] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@bulislaw @hugueskamba @kjbracey-arm

Release Notes

hugueskamba

LGTM. I will give others a chance to review before approving.

docs/design-documents/drivers/serial/serial.md

ciarmcom · 2019-10-04T17:24:14Z

@evedon, thank you for your changes.
@bulislaw @kjbracey-arm @hugueskamba @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

bulislaw

Great work, looks like we can remove a lot of complexity and maybe even save some memory. Could you guys make sure the debug output can still be redirected to https://github.com/ARMmbed/mbed-os/blob/master/drivers/SerialWireOutput.h with this new architecture.

docs/design-documents/drivers/serial/serial.md

hugueskamba · 2019-10-08T16:46:51Z

@evedon Are we missing a way to completely take out the re-targeting code if we do not need it?
How do I exclude the content of mbed_retarget.c if I am planning on using say UnbufferedSerial directly?

kjbracey · 2019-10-15T10:24:48Z

Silly question - what's the simplest way to view this as a whole with diagrams?

docs/design-documents/drivers/serial/serial.md

kjbracey · 2019-10-15T10:58:49Z

docs/design-documents/drivers/serial/serial.md

+
+![Retarget](./diagrams/retarget.png)
+
+The POSIX layer and the static array of `FileHandle*` allocated by the retarget code for file handling consume a lot of memory. A constrained target that only uses a console does not need file handling and should be able to turn this functionality off.


Also the virtual-ness of the FileHandle means ROM.

kjbracey · 2019-10-15T10:59:18Z

docs/design-documents/drivers/serial/serial.md

+1. Redefine the low-level library functions. A detailed description can be found [here](http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0475m/chr1358938931411.html)
+1. Redefine the system I/O functions.
+
+A minimal console only needs to write a single character at a time. So redefining the default fputc() to directly write to the serial port if the output stream is stdout and bypassing the system I/O functions should achieve higher memory savings. If we take this approach, we will have to rework some error handlers that rely on the POSIX form of `write(STDOUT_FILENO, buf, len)` to do emergency printing.


That method is also toolchain dependent - I don't know if we've established how easy it is with the other 2.

Indeed the first approach looks more complex. We should only pursue if it brings more memory saving. We'll decide during implementation.

docs/design-documents/drivers/serial/serial.md

kjbracey · 2019-10-15T11:03:05Z

@evedon Are we missing a way to completely take out the re-targeting code if we do not need it?
How do I exclude the content of mbed_retarget.c if I am planning on using say UnbufferedSerial directly?

Oh, completely remove it? Interesting. I would hope that if you never actually call any stdio functions, nothing would get pulled in anyway. I believe stdin/stdout are initialised on first use, not actually pre-main. Maybe need to turn off the "flush on exit" option or something?

But I can see some value in an option to make all definitions of the POSIX calls are minimal do-nothing stubs, even if there are lingering app calls.

kjbracey · 2019-10-15T11:03:49Z

This looks good, happy with almost all of it, main concern is about the value of MinimalSerial - would like to discuss a bit.

hugueskamba · 2019-10-17T12:15:24Z

Silly question - what's the simplest way to view this as a whole with diagrams?

@kjbracey-arm
Here: https://github.com/evedon/mbed-os/blob/serial_design/docs/design-documents/drivers/serial/serial.md

evedon · 2019-10-30T16:02:47Z

All points addressed and MinimalSerial removed as it did not bring memory saving. Please let me know if you have further comment.

docs/design-documents/drivers/serial/serial.md

evedon · 2019-11-01T11:08:12Z

Latest comments addressed in last commit. All commits squashed.

kjbracey · 2019-11-04T10:26:29Z

Couple more thoughts on default changes - I'm advocating for stdio-buffered-serial to default to true, but there are two other possible changes

stdio-convert-tty-newlines or stdio-convert-newlines to true. (I was pushing for this to be the case for 5.0, but didn't get it - there was a big "be compatible with Mbed 2" wish, kind of as a counterpoint to Mbed OS 3).
stdio-baud-rate to 115200.

I'm rather indifferent on the baud rate, but I know some others are keenly in favour. (It's possible that lack of buffering and hence unreliable input may have been part of the reason for running slow originally.)

But I'm continually annoyed by how much pointless \r\n I see in applications. All those \r take up ROM...

evedon · 2019-11-04T13:16:01Z

Couple more thoughts on default changes - I'm advocating for stdio-buffered-serial to default to true, but there are two other possible changes

stdio-convert-tty-newlines or stdio-convert-newlines to true. (I was pushing for this to be the case for 5.0, but didn't get it - there was a big "be compatible with Mbed 2" wish, kind of as a counterpoint to Mbed OS 3).

stdio-baud-rate to 115200.

I'm rather indifferent on the baud rate, but I know some others are keenly in favour. (It's possible that lack of buffering and hence unreliable input may have been part of the reason for running slow originally.)

But I'm continually annoyed by how much pointless \r\n I see in applications. All those \r take up ROM...

This design focuses on serial classes and I am reluctant to increase the scope by including configurations changes. Having said that, I agree that if we advocate in the documentation for BufferedSerial to be the default serial choice the we should make stdio-buffered-serial default to true.

For the other configuration changes, I am in favour of them but it is not part of this design. I created a JIRA ticket https://jira.arm.com/browse/IOTCORE-1423. We will try to address the ticket in this planning increment if there is time.

hugueskamba

LGTM

bulislaw

Looks good couple of nitpicks and questions about size impact.

bulislaw · 2019-11-04T13:43:35Z

docs/design-documents/drivers/serial/serial.md

+
+### Scenario 1 `Printing to stdout`
+
+There is no change in the current APi in regard to printing to stdout.  Applications should call `printf` directly as shown below. The console baud rate can be configured via `platform.stdio-baud-rate` in `mbed_app.json`.


I don't really like the should, I would say can and also add a small disclaimer, something like (it can negatively impact the image size)

Negatively impact compared to what? I believe this section started as a comparator to Serial::printf, and I think that's its focus. The example doesn't even need to be printf, it could have been puts.

If we want to talk about printf versus alternatives, that's at a bit of a tangent to the serial work, but I guess the possibilities are:

Using printf

Using printf with minimal-printf on, reducing formatter

Only using puts (or all your printfs are simple enough for compiler to turn them into puts), avoiding formatter

Using just write, avoiding C library altogether

Turning on minimal-console (in conjunction with any of the 4 previous options)

I meant calling stdlib printf from the application, and it's a good point it's not a place to talk about memory optimisations. But I don't want to push people to printf too much.

I changed should into can and put the example using write first. Discussion related to printf comes after so I am not emphasing the use of printf.

docs/design-documents/drivers/serial/serial.md

0xc0170 · 2019-11-05T10:26:18Z

docs/design-documents/drivers/serial/serial.md

+
+### Detailed design : BufferedSerial
+
+`BufferedSerial` is `UARTSerial` renamed to convey the original purpose of the class. In addition, the new class will allow configurability of the transmit and receive buffer size so that each instance of the class can configure its buffer size according to its intended usage, for example a modem may need a large input buffer and a console may not need any input buffer.


The name reminds me earlier component available on developer page. It's been used, I recall the name UARTSerial was chosen not to create a naming clash.
I assume they are not compatible and users would need to update their apps if they move to this new class? Might be just noted here

Indeed I can see references to a BufferedSerial class if I google it. But I don't think the class was even part of a Mbed OS release. Wouldn't it create even more confusion if we refer to a class not in Mbed OS source code?

As part of this work, we will deprecate Serial and RawSerial so users will get warnings if they use deprecated code (The BufferedSerial class publicly derived from RawSerial).

It has been used mainly in mbed 2 apps. Correct, was not part of the release. Just wanted to make sure we are aware. It shall be fine as it is.

docs/design-documents/drivers/serial/serial.md

bulislaw

LGTM

0xc0170 · 2019-11-07T10:33:10Z

CI started

mbed-ci · 2019-11-07T14:07:14Z

Test run: FAILED

Summary: 2 of 12 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test
jenkins-ci/mbed-os-ci_exporter

0xc0170 · 2019-11-08T09:31:39Z

Restarted CI (some internal fault , I could not restart just 2 stages)

hugueskamba reviewed Oct 4, 2019

View reviewed changes

docs/design-documents/drivers/serial/serial.md Outdated Show resolved Hide resolved

ciarmcom requested review from bulislaw, hugueskamba, kjbracey and a team October 4, 2019 17:24

ciarmcom added the needs: review label Oct 4, 2019

bulislaw reviewed Oct 8, 2019

View reviewed changes

kjbracey reviewed Oct 15, 2019

View reviewed changes

hugueskamba reviewed Oct 31, 2019

View reviewed changes

evedon force-pushed the serial_design branch from 7d81305 to 1fb71cc Compare November 1, 2019 10:58

hugueskamba mentioned this pull request Nov 1, 2019

mbed_retarget: Add a minimal console implementation to provide basic functionalities #11796

Merged

hugueskamba approved these changes Nov 4, 2019

View reviewed changes

bulislaw reviewed Nov 4, 2019

View reviewed changes

0xc0170 reviewed Nov 5, 2019

View reviewed changes

Serial design draft

813f680

evedon force-pushed the serial_design branch from 056f70d to 813f680 Compare November 5, 2019 17:55

bulislaw approved these changes Nov 7, 2019

View reviewed changes

0xc0170 approved these changes Nov 7, 2019

View reviewed changes

0xc0170 added needs: CI release-version: 5.14.2 and removed needs: review labels Nov 7, 2019

0xc0170 added ready for merge release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 and removed needs: CI release-version: 5.14.2 labels Nov 8, 2019

0xc0170 merged commit 6e6c7d0 into ARMmbed:master Nov 8, 2019

0xc0170 removed the ready for merge label Nov 8, 2019

0xc0170 added release-version: 5.15.0-rc1 and removed release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 labels Nov 19, 2019

evedon deleted the serial_design branch January 9, 2020 13:03


		![Retarget](./diagrams/retarget.png)

		The POSIX layer and the static array of `FileHandle*` allocated by the retarget code for file handling consume a lot of memory. A constrained target that only uses a console does not need file handling and should be able to turn this functionality off.


		### Scenario 1 `Printing to stdout`

		There is no change in the current APi in regard to printing to stdout. Applications should call `printf` directly as shown below. The console baud rate can be configured via `platform.stdio-baud-rate` in `mbed_app.json`.


		### Detailed design : BufferedSerial

		`BufferedSerial` is `UARTSerial` renamed to convey the original purpose of the class. In addition, the new class will allow configurability of the transmit and receive buffer size so that each instance of the class can configure its buffer size according to its intended usage, for example a modem may need a large input buffer and a console may not need any input buffer.

Serial design draft #11634

Serial design draft #11634

Uh oh!

Conversation

evedon commented Oct 4, 2019

Description

Pull request type

Reviewers

Release Notes

Uh oh!

hugueskamba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ciarmcom commented Oct 4, 2019

Uh oh!

bulislaw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hugueskamba commented Oct 8, 2019

Uh oh!

kjbracey commented Oct 15, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kjbracey commented Oct 15, 2019

Uh oh!

kjbracey commented Oct 15, 2019

Uh oh!

hugueskamba commented Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evedon commented Oct 30, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

evedon commented Nov 1, 2019

Uh oh!

kjbracey commented Nov 4, 2019

Uh oh!

evedon commented Nov 4, 2019

Uh oh!

hugueskamba left a comment

Choose a reason for hiding this comment

Uh oh!

bulislaw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

hugueskamba commented Oct 17, 2019 •

edited

Loading