Wire: Define WIRE_HAS_TIMEOUT #362
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I added one more commit that also defines the default timeout in Wire.h (without changing it). |
|
I added one more commit that renames a parameter to be a bit more clear, I noticed this when working on a documentation proposal. |
facchinm
approved these changes
Sep 21, 2020
|
|
|
Guys, something to think about is how to handle defaults in the Wire.cpp code. The chipKit platform handles this by maintaining a reference counter that gets bumped when begin() is called and decremented when end() is called and only does special initialization the first time begin() is called. For this platform, If it is desirable to only set the defaults once, out of begin() and into the constructor. That will allow it to only set the defaults once so that subsequent calls to begin() do not clobber it. |
|
Good point. I also agree with your proposed solution. Even though it is not always advisable to do initialization in the constructor of a global object, in this case, the initialization is really only just setting variables, so it should be safe. This does raise the question: Should maybe twi.c just have default values for these timeout variables instead, referring to these constants? That would be more efficient, though currently I don't think twi.c refers to Wire.h, so that would break the abstraction a bit (even though that distinction is not really useful atm anymore, but still). |
|
Good question. I assume that long term, that some sort of timeout support will be enabled by default so some of this is kind of more related to the transition to that point and for those of us that are worrying about backward compatibility issues. I guess it is basically a judgement call as to where the defaults should live. From an API and documentation perspective, it may make sense if Wire declares and documents its own defaults and uses them. I don't have very strong feelings about it. One thing I just noticed in Wire.h and the twi.c code is that it appears that are 3 different sets of defaults being used that are different.
And then this in the TwoWire class:
static volatile uint32_t twi_timeout_us = 0ul; |
Excellent point, though the code is currently not quite structured to really support this. I guess if the AVR core ever moves to this, maybe the Wire/twi distinction should be just removed anyway (are there any AVR chips with multiple I2C interfaces at all, though?)
Agreed, all of utility/twi is an implementation detail and should be covered by APIs in the Wire "namespace".
No, since the current defaults are no timeout, and the defaults for In any case, here's what I'm thinking to do: Let twi.h expose constants for the default values, which are then used in twi.c. Then Wire.h can redefine its own constants (this will be the publically usable API, and named like the constants in the current version of this PR), which are just initialized to the TWI constants, so the values match. Then, the call to
I'll go ahead and implement the above right away, let's see how that looks. |
In commit deea929 (Introduce non compulsory Wire timeout), some timeout-related functions were added. To allow writing portable sketches, it is important for those sketch to know whether they are using a Wire library version that is new enough to offer these functions without having to rely on version number checking (since other Arduino cores have their own versioning schemes). This adds a WIRE_HAS_TIMEOUT macro, similar to the existing WIRE_HAS_END macro, to facilitate that. This relates to arduino#42.
Previously, these were implicit in the default values of some global variables. Now, the default timeout is defined in twi.h and exposed through Wire.h, which: - Makes it easier to change later - Allows sketches to detect the default timeout value Note that this definition is split between twi.h and Wire.h to ensure that the default values are available as compile-time constants in twi.c (which allows for more efficient initialization than e.g. writing these values from the Wire constructor or from the begin() method, where the latter has the extra downside of potentially overwriting values previously set with setWireTimeout() if begin() is called again). Additionally, since twi does not currently depend on Wire.h, defining these values in Wire.h and using them in twi.c was not feasible without breaking adding this extra dependency, so instead this commit defines the values in twi and then re-exposes them in Wire.h (with the intention of sketches referring to the Wire.h versions, not the twi.h versions). See arduino#362 for additional discussion.
This parameter was recently added, but the name was not ideal. Renaming it would make it more clear, and also more consistent with the variable it eventually sets and the comments.
matthijskooijman
force-pushed
the
wire-has-timeout
branch
from
eadfd56 to
bc638e1
Compare
|
I just did a force push implementing the above strategy. Only commit d739b3f was changed, the others are unchanged (but everything is rebased on top of current master). As far as I'm concerned, this addresses the excellent concerns raised by @bperrybap and makes this PR ready to merge again. |
This method was renamed shortly before merging this code and it seems some comments were note updated. Fixes: arduino#353
|
I've done a force push to add a commit message reference to #353 (which is fixed by the last commit). Can we get around to merging this? I think having a reliable way to detect feature support is an important part of (this) API, and really should not be left unfixed... |
matthijskooijman
force-pushed
the
wire-has-timeout
branch
from
bc638e1 to
fd0b3ee
Compare
|
Memory usage change @ fd0b3ee
Click for full report table
Click for full report CSV |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In commit deea929 (Introduce non compulsory Wire timeout), some
timeout-related functions were added. To allow writing portable
sketches, it is important for those sketch to know whether they are
using a Wire library version that is new enough to offer these functions
without having to rely on version number checking (since other Arduino
cores have their own versioning schemes).
This adds a WIRE_HAS_TIMEOUT macro, similar to the existing WIRE_HAS_END
macro, to facilitate that.
This relates to #42.