Navigation Menu

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

drivers/periph/uart: add uart_write_byte() & uart_write_string() functions #15499

Closed
wants to merge 4 commits into from

Conversation

benpicco
Copy link
Contributor

Contribution description

When implementing the serial bootloader feature, I added some UART convenience functions.

  • uart_write_byte() to send a single byte over UART
  • uart_write_string() to send a string of text. This is especially useful for debugging with no stdio

Both are implemented as common fallback functions in drivers/periph_common but can be implemented by the CPU.
An implementation for sam0 is provided for uart_write_byte().

Testing procedure

Issues/PRs references

@benpicco benpicco added Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Nov 23, 2020
@aabadie
Copy link
Contributor

aabadie commented Nov 23, 2020

You may want to extend tests/periph_uart with these additional functions.

@benpicco
Copy link
Contributor Author

benpicco commented Nov 24, 2020

Good idea, that makes the code a bit nicer to read.

@benpicco benpicco added this to Backlog / Proposals in RIOT Sprint Day via automation Dec 1, 2020
* @param[in] data byte to write
*
*/
void uart_write_byte(uart_t uart, uint8_t data);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this simply be a static inline around uart_write?

Copy link
Contributor Author

@benpicco benpicco Dec 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, moved it there.

@benpicco benpicco removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 8, 2020
@fjmolinas fjmolinas moved this from Backlog / Proposals to Under Review in RIOT Sprint Day Dec 8, 2020
@fjmolinas fjmolinas self-assigned this Dec 8, 2020
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm somehow unsure about this PR, it cutters the API a bit against for some convenience functions that are actually quite simple, and already implementable with one liners uart_write(uart, (uint8_t*)s, strlen(s)); and uart_write(uart, &data, 1);

* This is a basic UART (Universal Asynchronous Receiver Transmitter) interface
* to allow platform independent access to the MCU's serial communication
* abilities. This interface is intentionally designed to be as simple as
* possible, to allow for easy implementation and maximum portability.

So I'm kind of looking to be convinced :) (but if someone else ACKs it I won't say anything either)

drivers/periph_common/uart.c Outdated Show resolved Hide resolved
@benpicco
Copy link
Contributor Author

benpicco commented Dec 8, 2020

Well unless an optimized version is to be provided, there is no additional implementation work to be done.

My original idea (besides convenience and nicer looking code) was that only using uart_write_byte in riotboot_serial might save a few bytes if the CPU provides a dedicated implementation.

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 16, 2021
@kaspar030
Copy link
Contributor

kaspar030 commented Feb 16, 2021

I'm also a bit unsure about this PR. Can we maybe split it:

  1. provide the two functions as plain wrappers around uart_write(), in periph_common (or just static inlines), without any way of overriding or providing optimized variants.

IMO, I think that should be good enough.

  1. See all the infra necessary for the optimized versions. This allows seeing the impact both on lines of code and possible savings.

@fjmolinas fjmolinas moved this from Under Review to In progress in RIOT Sprint Day Mar 30, 2021
@fjmolinas fjmolinas moved this from In progress to Backlog / Proposals in RIOT Sprint Day Mar 30, 2021
@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@benpicco benpicco removed this from Backlog / Proposals in RIOT Sprint Day Nov 29, 2021
@stale
Copy link

stale bot commented Mar 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 2, 2022
@stale stale bot closed this Apr 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR State: stale State: The issue / PR has no activity for >185 days Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants