arduino / ArduinoCore-API

Hardware independent layer of the Arduino cores defining the official API
https://www.arduino.cc/reference/en/
GNU Lesser General Public License v2.1
202 stars 117 forks source link

[Feature Request] Make Wire library methods virtual #31

Open simonnilsson opened 5 years ago

simonnilsson commented 5 years ago

I suggest making the public methods of the TwoWire class virtual in order to allow polymorphism when subclassing it.

Use case: Have another library that utilizes the Wire library and takes a refrence to a TwoWire instance in constructor. Want to use it on pins without hardware I2C (or other reason for not wanting to use the standard Wire implementation). So I want to pass another class instance that inherits from TwoWire and implements the same methods (but uses software I2C). This does not work with current implementation because of early binding (the base class functions will be called instead of the ones in subclass).

Moved from: https://github.com/arduino/Arduino/issues/8734

matthijskooijman commented 5 years ago

Note that there might be a significant performance impact (in particular, extra runtime, extra memory for vtables and possibly less opportunity for the compiler to optimize). With the newer compiler versions we're using now and lto enabled, gcc might be able to do devirtualization optimizations to remove the overhead when just using a single subclass, but that something that should be checked (since I know gcc can do it, but in the past it would only catch very obvious cases IIRC).

PaulStoffregen commented 5 years ago

Regarding devirtualization and other C++ optimizations, experience from Teensy has shown the compiler does much better when the class uses a constexpr constructor.

AxTheB commented 4 years ago

I have more concrete use-case for this request: I want to use more i2c devices with same hard coded address (in my case more than two of adaftruit's BME280). Adafruit's library supports it, the SoftwareWire library supports it (needs trivial splitting of few functions), and Wire.h is already halfway there - read and write (and few others) are virtual, beginTransmission, beginTransmission and requestFrom are not).

Testato commented 4 years ago

+1

SukkoPera commented 4 years ago

I absolutely second this request. I don't think it can cause any harm (the class is already virtual due to inheritance from Print) and it would open up a lot of possibilities.

For instance, this MCP23017 interface library has the possibility to use an alternative i2c library implementation, such as this one, but this feature currently cannot be used for any useful purpose due to the issue detailed here.

(Something similar should probably be done for the SPI library.)

bxparks commented 3 years ago

See https://github.com/arduino/ArduinoCore-avr/pull/331 where it was merged, then reverted, due to significant increase in flash memory consumption, just as @matthijskooijman warned above.