-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Declare TwoWire functions as virtual #396
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Declare TwoWire functions as virtual #396
Conversation
To make alternative implementations of the TwoWire class (e.g. SoftwareWire for software I2C) work properly being passed to libraries that expect TwoWire type.
@aentinger could you please approve? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
This causes flash memory to increase by ~650 bytes, and static RAM to increase by 42 bytes on AVR processors. If SoftwareWire does not require every method to be virtual, a more surgical approach might be preferable. Tested on:
|
This ⬆️ is "a bit" dangerous, and it could be prevented by adding the CI for code size on this repo too. @per1234 do you have some time to create a PR? Even without external libraries, just the bundled ones. |
Good by me, I'll revert it. |
You got it! #413 We could increase the core library coverage without introducing too much in the way of external dependencies by adding in compilation of the built-in examples, as was done in the sketch compilation workflows for the megaAVR and SAMD platforms . |
Even though this PR was reverted, I want to give folks like @ArkadyGamza, @ahnolds (#331) and @SRGDamia1 (#94) some hope about using So instead of using template <typename WIRE>
class MyClass {
public:
explicit MyClass(WIRE& wire) : mWire(wire) {}
void sendInfo() {
mWire.beginTransmission(address);
mWire.write(...);
mWire.endTransmission();
...
}
private:
WIRE& mWire;
};
// Use <Wire.h>
MyClass<Wire> instanceUsingWire(Wire);
// Use <SoftwareWire.h>
SoftwareWire softwareWire(SDA, SCL);
MyClass<SoftwareWire> instanceUsingSoftwareWire(softwareWire); I did a quick test of this with one of my programs, and got it working with both One final comment about |
IMO, doing something like this (making all functions virtual) is not the solution to the issue. In the bigger picture I think how to update/modify things to better support multiple Wire/i2c libraries, multiple Wire/i2c objects for multiple i2c buses needs quite a bit of thought and discussion. Seems like something for a good discussion on the mailing list? The issue I see for this type of solution, is that it is attempting to try to resolve an issue that is outside the library being updated/modified. Also, does everyone understand that virtual functions are always linked in regardless of whether they are used or not? IMO, the proper way to fix this is to update the libraries that use Wire services, after all it is their issue.
|
Fix #94
To make alternative implementations of the TwoWire class (e.g. SoftwareWire for software I2C) work properly being passed to libraries that expect TwoWire type.