drivers: serial: return int value from uart_poll_out #64081

martinjaeger · 2023-10-18T13:24:52Z

With the previous return type void some drivers silently dropped characters without any possibility for a user of the API to notice whether a character was transmitted successfully or not.

This is not a breaking change of the stable API, as existing code (ignoring the return value) will continue to work as before.

With the previous return type void some drivers silently dropped characters without any possibility for a user of the API to notice whether a character was transmitted successfully or not. This is not a breaking API change, as existing code (ignoring the return value) will continue working as before. Signed-off-by: Martin Jäger <martin@libre.solar>

This avoids compiler warnings because of unused return values. Signed-off-by: Martin Jäger <martin@libre.solar>

aaillet

LGTM, renesas part

nordicjm

auxdisplay, MCUmggr changes OK

nordicjm · 2023-10-19T07:36:31Z

drivers/serial/uart_rtt.c


-	SEGGER_RTT_Write(ch, &c, 1);
+	return ret > 0 ? 0 : -EIO;


>=? RTT can have a buffer depending upon Kconfig settings so I don't think it's right for 0 to return an error here

As far as I understood, 0 means that the character was dropped and not written into the buffer, which should be indicated with an error, no?

https://wiki.segger.com/RTT#SEGGER_RTT_Write.28.29

>= 0 | Number of bytes which have been sent.
< 0 | Error.

Interesting. How can an unsigned function return a negative error code?

In any case: If I want to send 1 byte, but 0 bytes "have been sent", that's an error in my understanding.

nordicjm · 2023-10-19T07:38:50Z

drivers/console/uart_mcumgr.c

@@ -206,7 +206,7 @@ static int uart_mcumgr_send_raw(const void *data, int len)

 	u8p = data;
 	while (len--) {
-		uart_poll_out(uart_mcumgr_dev, *u8p++);
+		(void)uart_poll_out(uart_mcumgr_dev, *u8p++);


@de-nordic will need to add code to bail at this point for 3.6

nordicjm · 2023-10-19T07:39:44Z

tests/drivers/uart/uart_basic_api/src/test_uart_poll.c

@@ -47,9 +47,9 @@ static int test_poll_out(void)
 		return TC_FAIL;
 	}

-	/* Verify uart_poll_out() */
+	/* Verify (void)uart_poll_out() */


comment update seems superfluous

jfischer-no · 2023-10-19T08:07:45Z

@martinjaeger This is an incompatible change to the OOT UART drivers, I guess you have to follow Introducing incompatible changes

carlescufi · 2023-10-19T08:31:50Z

This is not a breaking change of the stable API, as existing code (ignoring the return value) will continue to work as before.

Well, it may introduce warnings to existing code?

martinjaeger · 2023-10-19T09:10:57Z

I didn't have OOT drivers in mind. I agree with @jfischer-no and @carlescufi that it has to go through the stable API change process. Will prepare RFC issue etc.

de-nordic · 2023-10-19T09:50:32Z

This is not a breaking change of the stable API, as existing code (ignoring the return value) will continue to work as before.

Well, it may introduce warnings to existing code?

There are (void) casts added in bulk, which is not ok, IMHO, as it just does +1 to "happy debugging!" in the future.

martinjaeger · 2023-10-19T10:36:13Z

There are (void) casts added in bulk, which is not ok, IMHO, as it just does +1 to "happy debugging!" in the future.

The idea is to make the API change as unintrusive as possible. The (void) casts keep the behaviour in the subsystems exactly as it was before. Future users of the poll out API can decide if they want to consider the return value or ignore it.
Why do you think this could lead to additional bugs?

de-nordic · 2023-10-23T11:47:06Z

There are (void) casts added in bulk, which is not ok, IMHO, as it just does +1 to "happy debugging!" in the future.

The idea is to make the API change as unintrusive as possible. The (void) casts keep the behaviour in the subsystems exactly as it was before. Future users of the poll out API can decide if they want to consider the return value or ignore it. Why do you think this could lead to additional bugs?

You will have the calls keep on silently failing even though you have added a way to inform about problem. IMHO you have provided fix for a problem, to then unfix it with cast.

The problem is that some of code, for example in MCUboot console_out, has been build around the uart_poll_out that can not fail, so now, even though you consider it to behave the same, this is no longer true as we now acknowledge possible failure and need to rework the code to address such possibility. In short: the API change adds possible bug everywhere we do not check the code returned by the function.

So we either have to wave every (void) cast line as ok or log potential bug for it.

carlescufi · 2023-10-30T12:03:23Z

@martinjaeger is the RFC issue ready?

martinjaeger · 2023-10-31T08:22:20Z

@martinjaeger is the RFC issue ready?

No, not yet. Got distracted with other higher priority work. I aim at getting RFC and PR update done within the next few days.

github-actions · 2023-12-31T00:29:53Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

martinjaeger · 2023-12-31T10:10:07Z

Unfortunately, I don't have enough free cycles to finish this up at the moment. Feel free to pick the commits if anyone likes to go through all in-tree users of the API to consider the return values.

martinjaeger force-pushed the uart-poll-out-return-value branch 4 times, most recently from 40d3e90 to 6b1617e Compare October 18, 2023 19:23

martinjaeger marked this pull request as ready for review October 18, 2023 20:06

martinjaeger requested review from sjanc, jakub-uC, nordic-krch, de-nordic, nordicjm, ceolin, loicpoulain, rlubos, tbursztyka and jukkar as code owners October 18, 2023 21:11

martinjaeger added 2 commits October 19, 2023 08:37

treewide: add (void) before all uart_poll_out calls

1baf801

This avoids compiler warnings because of unused return values. Signed-off-by: Martin Jäger <martin@libre.solar>

martinjaeger force-pushed the uart-poll-out-return-value branch from 1f1bb53 to 1baf801 Compare October 19, 2023 06:38

aaillet approved these changes Oct 19, 2023

View reviewed changes

nordicjm reviewed Oct 19, 2023

View reviewed changes

dcpleung added the treewide 🧹 label Oct 19, 2023

carlescufi added DNM This PR should not be merged (Do Not Merge) Breaking API Change Breaking changes to stable, public APIs labels Oct 23, 2023

github-actions bot added the Stale label Dec 31, 2023

martinjaeger closed this Dec 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: serial: return int value from uart_poll_out #64081

drivers: serial: return int value from uart_poll_out #64081

martinjaeger commented Oct 18, 2023 •

edited

Loading

aaillet left a comment •

edited

Loading

nordicjm left a comment

nordicjm Oct 19, 2023

martinjaeger Oct 19, 2023

nordicjm Oct 19, 2023

martinjaeger Oct 19, 2023

nordicjm Oct 19, 2023

nordicjm Oct 19, 2023

jfischer-no commented Oct 19, 2023

carlescufi commented Oct 19, 2023

martinjaeger commented Oct 19, 2023

de-nordic commented Oct 19, 2023

martinjaeger commented Oct 19, 2023

de-nordic commented Oct 23, 2023

carlescufi commented Oct 30, 2023

martinjaeger commented Oct 31, 2023

github-actions bot commented Dec 31, 2023

martinjaeger commented Dec 31, 2023

drivers: serial: return int value from uart_poll_out #64081

drivers: serial: return int value from uart_poll_out #64081

Conversation

martinjaeger commented Oct 18, 2023 • edited Loading

aaillet left a comment • edited Loading

Choose a reason for hiding this comment

nordicjm left a comment

Choose a reason for hiding this comment

nordicjm Oct 19, 2023

Choose a reason for hiding this comment

martinjaeger Oct 19, 2023

Choose a reason for hiding this comment

nordicjm Oct 19, 2023

Choose a reason for hiding this comment

martinjaeger Oct 19, 2023

Choose a reason for hiding this comment

nordicjm Oct 19, 2023

Choose a reason for hiding this comment

nordicjm Oct 19, 2023

Choose a reason for hiding this comment

jfischer-no commented Oct 19, 2023

carlescufi commented Oct 19, 2023

martinjaeger commented Oct 19, 2023

de-nordic commented Oct 19, 2023

martinjaeger commented Oct 19, 2023

de-nordic commented Oct 23, 2023

carlescufi commented Oct 30, 2023

martinjaeger commented Oct 31, 2023

github-actions bot commented Dec 31, 2023

martinjaeger commented Dec 31, 2023

martinjaeger commented Oct 18, 2023 •

edited

Loading

aaillet left a comment •

edited

Loading