Don't hide I2C error status.

[dok-net]

Prior to this PR, most functions in the MCP23017 API fail silently, despite the I2C Wire libs error status return code.
Now, like other I2C slave driver libs, the return type is bool instead of void, so the user can determine the success of the operations.

[blemasle]

Thanks for the suggestion. I'm sorry but for the time being this is not a direction I'm willing to take for that lib for several reasons. They're all inter-twinned, so I'm sorry if this explanation goes back and forth 🙂

• This lib assumes no error occurs during transmission. It is not an oversight but a design choice that keeps the lib very simple, small and straightforward.
• Doing otherwise would require way more modifications than you proposed to be coherent. For instance pinMode, portMode or readRegister should probably stop on the first encountered error and return that failing status. In fact, every function would need to return an error code/status, not just the subset you modified. Error control should then be implemented everywhere and would probably never satisfy every use case because the MCP can be left in an unpredictable state. Trying to do so is a waste of time and hex space for a very minor benefit. Implementing this would add way too much plumbing for what I perceived as a very niche use case.
• I made quite a heavy usage of the MCP23017 in some unpublished projects and never encountered transmission issues while using multiple MCP on the same bus along with other chips. The only times I had issues were in the end because of some wiring issues and/or missing pullups on the SCL/SDA lines. The way to debug these is not by using this lib, and should be sorted out beforehand.
• Some functions already needs to return a value (eg readRegister, see comment below). In that case the error status cannot be returned, apart from using references / pointers which would made the whole usage cumbersome if you do not need error checking (again, a niche use case from my point of view). Errors checking is irrelevant/untrustworthy if you can only do it partially (eg on writeRegister but not on pinMode). And doing it one way on some functions and differently on another is not really an option nor a seal of quality.
• In the case of an error on a multiple commands function, the device is left in an unpredictable state and I don't think you can blindy "retry" your commands. Again, if you have a transmission error at some point you probably have something going on on the hardware side (or how you're chaining those commands) and you should try to fix this rather than using some sort of retry loop on the software side.

As you can probably tell, I'm not really into those changes. 😁 I might be biased here, but as I said, the current pattern is not an oversight but a design choice intended to keep the lib simple, small and consistent.
As I see it it would requires a lot more work than the few lines you modified to be relevant, coherent and useful. On the useful part (because it would be the driving reason to do it after all), I'm sorry but I do not see it. The few real use cases I see would be for debugging, which are not the concern of this lib.

I'm not closing this for now, don't hesitate to comment further if you have other points/arguments that tend to show that those changes could be useful for everyone 🙂.

[blemasle]

retval isn't and cannot be used here as readRegister needs to return its own value.

[blemasle]

Again, you've added a line instead of modifying one.
Besides that, if there is an error here, continuing the execution would have no sense have I wouldn't even know what have been read 🤔

[blemasle]

You've added a line instead of modifying one.

[dok-net]

I'm sorry, the PR obviously isn't finished yet. I missed that over a weekend or something. Still good to have your views on the matter in general. I think I will improve the patch until all functions that are still void return an error flag. Other libs for I2C devices face the same ambiguity between the functions that can have an error return, and those that return values when successful.
Perhaps we could agree on adding an error state to the class which can be polled, and closes the gap on the functions that cannot return the state directly? It's all non-reentrant anyway.
I don't agree that it's for debugging the software, because there can be hardware problems that the software developer otherwise might miss for awhile. So hiding error conditions is never a good idea, even doing it imperfectly is better that no error reporting at all.