Skip to content

Conversation

@maidnl
Copy link
Contributor

@maidnl maidnl commented Sep 23, 2025

This PR takes care of issue #492 .
There was a default timeout of 1000 ms in I2C communication.
Timeout has been reduced to 1ms.
However (although this is not a standard Wire function) a setTimeout() function has been added to Wire class so that users can tune the value.

@per1234 per1234 added the topic: code Related to content of the project itself label Sep 23, 2025
@maidnl
Copy link
Contributor Author

maidnl commented Sep 24, 2025

@per1234 I renamed the function according to the request, however please note that in the current implementation of Wire (at least in Core-Renesas) the Wire class is derived from HardwareI2C class, that is derived from Stream. And Stream has already setTimeout function.
void setTimeout(unsigned long timeout); // sets maximum milliseconds to wait for stream data, default is 1 second
In my opinion it is better to use setTimeout name which is in line with the current class derivation scheme.

@per1234
Copy link
Collaborator

per1234 commented Sep 25, 2025

Stream has already setTimeout function.

That function has a different purpose from what is being implemented here.

Stream::setTimeout sets the timeout for the high level blocking functions of the Stream class (e.g., readBytes).

The function implemented here is setting a timeout for the low level communication. This is the reason why the function was intentionally given a distinct name in the AVR library, rather than shadowing the Stream class member, which would have made it impossible to set the Stream timeout.

@per1234 per1234 dismissed their stale review October 10, 2025 05:16

Requested change has been made. Thanks!

Copy link
Collaborator

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

As @HanzHager reported in #497 (comment), the parameter of the established setWireTimeout implementation is the timeout duration in microseconds:

https://docs.arduino.cc/language-reference/en/functions/communication/wire/setWireTimeout/#parameters

timeout in microseconds, if zero then timeout checking is disabled

However, in your proposal the argument is instead a timeout duration in milliseconds.

Please bring the function into compliance with the established standardized API for Wire library implementations.

@HanzHager
Copy link

  • setWireTimeout(0,false) should disable timeout.

  • the new PR changes the default timeout value from 1000ms to 25ms (as also my PR does). This is not fully backwards compatible.
    Maybe ok to keep 1000ms when setWireTimeout() is introduced, unless it is obvious that 1000ms is a bug.

  • setWireTimeout(), without parameters sets another arbitrary timeout value than the default one and it is not obvious what the default is and what the initial default value is
    In the full solution for setting the timeout, there are macros that could be used to show what the default value is.

  • define WIRE_DEFAULT_TIMEOUT to be the default timeout value

@maidnl maidnl force-pushed the i2c_empty_bus_timeout branch from d1c00ea to 6107e3f Compare December 1, 2025 11:49
@maidnl maidnl force-pushed the i2c_empty_bus_timeout branch from 6107e3f to 809c48e Compare December 1, 2025 13:39
@facchinm
Copy link
Member

@maidnl it might be worth trying to find the minimum timeout that doesn't trigger #520 but also allow the scan to finish in a reasonable time in case the bus is empty

@HanzHager
Copy link

I think that it might be a good idea to keep the old default value, for backwards compatibility.
Even if the problem in these reported cases are probably due to problems in the hardware implementation, if the worked with the old timeout value it is breaking things without adding value when changing the default value in the platform.
For the cases where the timeout needs to be less than 1000ms, it makes sense to use the setWireTimeout() that is added with this PR.
Still, I advocate crating a full implementation of https://docs.arduino.cc/language-reference/en/functions/communication/wire/setWireTimeout/#parameters so that the user has flexibility to inspect and change the timeout value in a way that is compatible with other implementations and the reference documentation.
See more details in my comment from Oct 13, 2025.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic: code Related to content of the project itself

5 participants