UnbufferedSerial: add is_tx_active()

[pennam]

Summary of changes

in certain situations, like #14227, using UnbufferedSerial we need to know if serial transmission ended before going on. This pr adds is_tx_active() method to UnbufferedSerial class.

/cc @facchinm

Impact of changes

Migration actions required

Documentation

---

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[X] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

---

Test results

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

---

Reviewers

@0xc0170

---

[ciarmcom]

@pennam, thank you for your changes.
@0xc0170 @ARMmbed/mbed-os-core @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-maintainers please review.

[ithinuel]

On the one hand the PR in itself looks Ok to me.
On the other hand I am not sure this is good to bring it that or at least this way in the API.
This method has a high potential of creating TOCTOU issues that would be hard to debug.

Since it's at the C++ level, it might be interesting to have a method that takes a lambda to prevent further TX to be started in a thread/interrupt safe manner.

@pan- Is there a c++ pattern that would help us dealing with this?

[pennam]

@ithinuel thanks for reviewing this. Just to better clarify the use case of is_tx_active() method; this is meant to be used not only to prevent further tx to be started but also to prevent other actions such as GPIO state changes.

For the particular case of #14227 this can be also changed in someting that waits the end of serial transmission and not just reporting the state, but maybe it becomes too specific.

If there is any suggestion on how to make this more safe i'm happy to implement and test it.

[pan-]

Overall I think the PR is OK with the current API however we should seriously consider fixing write to return when the last byte in the buffer has been transmitted. Some signal indicating that the internal UART buffer is empty could be nice too.

[pan-]

You don't need to reimplement the method. Just import it from the parent class:

using SerialBase::is_tx_active;

[kjbracey]

consider fixing write to return when the last byte in the buffer has been transmitted.

That's a bit extreme, and would have real-world performance impact. Normal users want their bytes going out back-to-back to maintain line rate, and you want at least one byte of data register/FIFO to do that, and have the next byte ready. "Unbuffered" means "no software buffer", not "sync the hardware FIFO after every write".

It would make sense to wire up FileHandle::sync() to drain output as much as you can - be the blocking form of this mechanism. I'm not sure why this hasn't already been done.

UnbufferedSerial currently only has this as a no-op.

BufferedSerial drains its software buffer, but doesn't wait for the HAL.

Both could be modified to spin waiting for serial_tx_active() to return false (I'd make that SerialBase::sync()). Can't see any reason not to do it apart from backwards compatibility paranoia.

[pan-]

Thanks @kjbracey-arm That would definitely make sense to wait for serial_tx_active to return false in FileHandle::sync().

[pennam]

@kjbracey-arm @pan- Thanks a lot for your suggestions. I will prepare another pr adding HAL synchronization with serial_tx_active into SerialBase::sync(). In the meanwhile i leave this open for reference.

[kjbracey]

Thanks a lot for your suggestions. I will prepare another pr adding HAL synchronization with serial_tx_active into SerialBase::sync(). In the meanwhile i leave this open for reference.

Cool. Note that even if you put sync into SerialBase, UnbufferedSerial will still need to explicitly implement UnbufferedSerial::sync itself calling that to override FileHandle::sync. See #14175 for a similar case and discussion - the issue is that SerialBase does not implement FileHandle, so its methods don't act as overrides, A using directive in UnbufferedSerial isn't sufficient to act as the virtual FileHandle override, unlike what you did for is_tx_active.

BufferedSerial obviously already implements sync for its software buffer, so you'd just be adding a call to SerialBase::sync at the end of that.

[ciarmcom]

This pull request has automatically been marked as stale because it has had no recent activity. @pennam, please carry out any necessary work to get the changes merged. Thank you for your contributions.

[pennam]

i'm closing this pr because i think further improvements should rely on #14600