Fix uart tx test

[LMESTM]

2 commits in this pull request:
1 fix for STM32 platforms
1 extra test for UART tx complete transmission

[LMESTM]

Adding reports. Report for non regression of Tx complete fix on ST:
reports_TxCompleteTesting.zip
Report on MBED39 test runs
report_Mbed39.zip

[bridadan]

@mbed-bot: TEST

HOST_OSES=ALL
BUILD_TOOLCHAINS=ALL
TARGETS=ALL

[mbed-bot]

[Build 411]
FAILURE: Something went wrong when building and testing.

[LMESTM]

how can I check what's wrong with continuous-integration/mbedci ?

[LMESTM]

@bridadan how can I check what's wrong with continuous-integration/mbedci ?

[bridadan]

Looks like there's a stray ' after Expected

[LMESTM]

Good catch - I'll solve this if we agree the test is useful

[LMESTM]

I've rebased the fix, so corrected also the ' typo

[bridadan]

Hi @LMESTM sorry for not getting back to you sooner. continuous-integration/mbedci is our private CI bot, so unfortunately you can't view the results publicly. Usually the failure is caused be existing issues in the code base, so it's not necessarily a problem with your PR.

Though in this case, it looks like a good amount of platforms aren't passing your Serial Complete test. Am I right in saying that the putc function is now completely blocking for the targets that you've implemented in this PR? Meaning the putc function won't return until the character has left the buffer?

I'm not sure if that's something we actually want to do by default. Though I think having a function to see if the serial tx is complete would be very useful to have, or to use this function during a blocking buffer flush.

What do you think, @0xc0170 ?

[LMESTM]

HI @bridadan and @0xc0170

I think the putc function is supposed to be a synchrnonous API, meaning that it will return only when complete, otherwise this may lead to unwanted behaviors as shown in test example (printf then sleep is supposed to sleep only after sending the whole printf right ?)

I agree that this is not efficient, (also not efficient to send byte by byte BTW). So I guess that's the reason for the ongoing implementation of asynchronous serial APIs with a callback to tell you when it's complete.

[bcostm]

OK with me to add the check of the "end of transmit" after each byte.

[LMESTM]

@bridadan @0xc0170
Hi again: the test is pretty basic: printf then sleep. I think mbed target should be reliable on this.
I've rebased on top of master. If you still think that the test is problematic, I can split this in 2 separate pull requests so that we can get STM32 commit applied and then discuss the new test separately.

[0xc0170]

tab used here? (misaligned)

[LMESTM]

Sorry about that - Solved now - I changed my editor settings since then

[0xc0170]

If you still think that the test is problematic, I can split this in 2 separate pull requests so that we can get STM32 commit applied and then discuss the new test separately.

Please do split those, remove from here the new test, and send new PR. Thanks

[LMESTM]

@0xc0170 done as proposed. New PR (and issue) for the new test are on the way

[LMESTM]

@0xc0170 @Sissors @adamgreen please consider this patch for merging.

This may have some impact on performances as long as sending bytes by bytes over serial, but this is the only way to be safe and is mandated in ST reference manual:
" After writing the last data into the USART_DR register, it is mandatory to wait for TC=1
before disabling the USART or causing the microcontroller to enter the low-power mode"
The performance aspect is addressed by ASYNC API which allows to send a buffer of data instead of single bytes and which gets the TC (Transmission Complete) flag with an int'errupt -s o no CPU load waste.

[adamgreen]

" After writing the last data into the USART_DR register, it is mandatory to wait for TC=1
before disabling the USART or causing the microcontroller to enter the low-power mode"

Of course a person could also argue that it mandates a change in the sleep API and not the serial API. The sleep API could be modified to wait for any enabled UARTs which have data yet to transmit.

[LMESTM]

@adamgreen
yes that's right, this is a possible evolution. If the sleep API later evolves to let drivers like UART inform sleep code whether sleeping is safe or not, then we can change the serial code again.
For now, I'd like to have the tests passing ok with the available fix. What do you think ?

[bridadan]

@LMESTM I would like to propose a different option.

Could we instead use your Serial Complete functions you've added and use those to properly implement a blocking buffer flush function (fflush)? That way we don't change the existing implementation but we provide a mechanism to explicitly block until the buffers are empty?

[0xc0170]

The sleep API could be improved, in some cases you can just provide some time for the system to respond (there are use cases where you don't know how long the packet/data transfer can take) or just check what is ongoing within a system).

You can provide to check if UART has finished into sleep API, that would be much better than to affect every putc write. Do all these target provide FIFO? I would assume some do, and putc could take an advantage of those aka push as much as you can to the hw. Sleep would wait for FIFO to get empty if sleep mode which is going to get activated do not keep serial active.

Let me know what do you think. I'll close this PR.

[0xc0170]

One addition: this discussion pinpointed there is a potential for a status function. Had a quick look at cmsis-driver which provides one https://www.keil.com/pack/doc/CMSIS/Driver/html/group__usart__interface__gr.html#ga1e8fdd54294b587438b2b72f4dbde004 . we introduced something similar to asynch api (serial_active()), which is not exactly the status. I imagine a status would return that tx is still transmitting (fifo not empty, or shift register). And sleep would use this API.

Please @LMESTM create an issue describing this, we would consider this for future.

[LMESTM]

All, I'd have to admit I'm disappointed that this thread is closed before we get an agreement.
I'd be fully happy to reject my proposal if there is another PR available on the table - but it's not the case. We're leaving now our STM targets in an unsteady state with only vague ideas on how to solve the problem - I have other activities ongoing and don't have time now to look at your proposals now whereas my PR was validated internally and does not cause any regression.

[LMESTM]

Also your proposal on sleep will not cover cases like serial change of baudrate, serial deinit, change of system clock, etc ...
So a note to whoever will make the changes later (maybe myself when I'm done with ongoing dev), many more changes are required in serial on top of the proposed ones in sleep to make sure we check the Tx is complete.

[0xc0170]

Also your proposal on sleep will not cover cases like serial change of baudrate, serial deinit, change of system clock, etc ...

Yes, this must be handled by an application at the moment (sleep just goes to sleep for most platforms). I don't understand how sleep is connected to change of baudrates, etc. We were discussing the TX complete action for stdout serial.

All, I'd have to admit I'm disappointed that this thread is closed before we get an agreement.

You can reopen it anytime, I closed this as 3 people agreed the serial putc should not have this behavior. you can push more commits/rebase it and reopen. It's not locked, just closed.

[LMESTM]

I don't understand how sleep is connected to change of baudrates, etc. We were discussing the TX
complete action for stdout serial.

Not only. My PR is for any generic usage of putc in serial_api.c I would expect that anyone could use it, not only for stdout.
It is not related only to sleep, I created the sleep example because is sounded a good example to me. But many other tests will fail, like
MBED_38 which contains sequence::
pc.printf(); // could be pc.putc()
del pc:
Another test that would fail is:
pc.printf(); //could be pc.putc()
pc.baud(change_rate);

So someone implementing a protocol over serial (BT chipset or any other ..) on a target that doesn't support ASYNC API, may face some of the issues mentioned above.

I don't know how to re-open this, but please consider this PR, even ifas a temporary fix until a more optimized solution is found and replaces this one. I'd really prefer a stable solution in place for now.

I rebased yesterday, can rebase again any time.

[bridadan]

@LMESTM Instead of modifying the behavior of the existing apis, would providing a function to explicitly wait while the buffers are emptied be a solution? We could augment the current tests to use this function.

This kind of function has been discussed before but I don't think anyone has looked in to how it would be implemented. There are functions in the standard library (ex. fflush, or perhaps sync) that allow you to flush buffers, but I don't think these functions work correctly at the moment. Perhaps it would be possible to enable these functions with the HAL support you've added here?

EDIT: for example

pc.printf(); // could be pc.putc()
// Block until all data sent
fflush(//file handle associated with pc) // could be more appropriate standard function (sync?)
del pc;

pc.printf(); //could be pc.putc()
// Block until all data sent
fflush(//file handle associated with pc) // could be more appropriate standard function (sync?)
pc.baud(change_rate);

[LMESTM]

@bridadan As far as I see that would make the changes and possible impacts on applications even more complex. The change I have proposed does not make any visible impact on API behavior and was tested ok on the complete STM32 family (and does not impact other targets).

To all:
I actually don't understand why this proposal raises so much concern ...

Let me try to clarify if I was not clear enough before.
The STM32 FIFO is only 1 byte, so the performance impact of my patch on applies on the last byte of the stream / string being sent.
Basically _ before my proposed patch _ most of the CPU time is being spent looping in:
while (!serial_writable(obj));
and _ after my proposed patch _ most of the CPU time is being spent looping in:
while(!serial_TxComplete(obj));
In every case, I think that the CPU is being wasted sending debug data over a slow interface.

The first thing that I would suggest to anyone which faces performances issue is : do not heavily rely on printf over serial as the main debug purpose - there are plenty of good debuggers supported with mbed. For the debug that you want to keep, do not hesitate to switch to at least 115200kbps instead of 9600.

[Sissors]

Sure you can use a debugger, but the point of the online compiler is that you don't need to install a tool chain and hope it works. And for debuggers you always need those, while printf is universal for every MCU that is mbed supported (okay, almost all).

The issue is that every single target implements putc non-blocking. It makes sense to me to have an API function to see if it is done transmitting, but by making it blocking you are simply not following what every other target does, and as a result you break libraries such as BufferedSerial and actually pretty much every single library that uses TX interrupts. Of course you can say you can also use the async API, but besides my personal opinion that that API has some issues, there is still the problem that it will mean quite some programs won't work on STM devices anymore and/or are alot slower. Even simple code with some serial passthrough/loopback could run into issues.

And as you say your FIFO is only 1 byte, so even without an API change the worst something would need to do is add wait(10.0f/9600) before going into deepsleep.

[LMESTM]

@Sissors :

as a result you break libraries such as BufferedSerial and actually pretty much every single library that uses TX interrupts.

I frankly missed that I would break anything with my patch.
I'll make a trial with BufferedSerial and check the impact of my proposed patch.
So we can for now keep the PR closed - sorry for the noise.

And as you say your FIFO is only 1 byte, so even without an API change the worst something would need to do is add wait(1/9600) before going into deepsleep.

Ok let's add up delays here and there, starting with tests like MBED_38 (and MBED_39 if merged) ...

[Sissors]

(Btw technically BufferedSerial and the likes probably still work, however they go from running interrupt based on the background, to permanently blocking everything from inside an interrupt until it is done sending.)

And just to be clear, overall I definately agree with you it would be good to have a method to know when it is save to change baudrate, enter deepsleep, etc. That is a good point, I just don't think making putc blocking is the answer.