Return the write status from the IO expander virtuals - #328
Merged
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
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.
Problem
#320 and #322 gave
setPullModeand the PWM methods failure reporting, butthe remaining
IOExpander_Basevirtuals —setDirection,setHighImpedance,digitalWrite,resetIrq,disableIrq,enableIrq— still returnvoid,so an I2C failure or an invalid pin is silently indistinguishable from
success. #320 explicitly left making them consistent for a separate change;
this is that change.
Fix
All six now return
boolwith the same contract assetPullMode:trueonly when the requested state was completely established,
falsefor aninvalid pin or an I2C failure. The implementations simply propagate the
status of the underlying register access that they already performed; no
register traffic changes. On the PI4IOE5V6408 the read-to-clear
resetIrqmoves from
readRegister8(which cannot report failure) toreadRegister,performing the same one-byte read.
Existing calls stay source-compatible since the result can be ignored; the
callers that must act on failures already do so since #326.
Breaking change
Only classes deriving from
IOExpander_Baseare affected: overrides writtenagainst the old
voidsignatures no longer compile and must change theirreturn type to
bool. Ordinary callers are unaffected.Post-review amendment
A local adversarial review (three independent passes) found that the
PI4IOE5V6408 setters accepted out-of-range pins —
1 << pincollapses to azero mask for pins 8-31, so the helper rewrote the register unchanged and
returned true, contradicting the documented contract (and shifting by 32+ is
undefined behavior). They now reject
pin >= 8likesetPullModealreadydid. The
@returndocs were also reworded to promise what theimplementations actually do — every required register access acknowledged —
rather than "state completely established": the written value is not read
back, and read-modify-write accesses are not atomic against concurrent
access to the same register.
M5IOE1_Class::setHighImpedancenow documentsthat it selects open-drain drive mode rather than disconnecting the pin.