Deriving from TwoWire pointless?

[SukkoPera]

I have tried to use this library. It's an interface library to a MCP23017 IC and I chose it because it allows passing a reference to an arbitrary TwoWire-derived library to be used for i2c communications. Of course the idea was to use it with your SoftwareWire library, as it is the only library of its kind that derives from TwoWire, which is a great idea!

The library saves a reference to the passed object internally. This reference must of course be of type TwoWire&. It is then used every time i2c bus operations are to be performed.

So does all of this work? Well, unfortunately it would be too good to be true :(. The problem is that a lot of the methods that you had to override (let's just consider begin/endTransmission() for instance) are NOT declared virtual in the base TwoWire class. This means that every time they are called through a pointer/reference to the base class (as the MCP23017 library does), the original implementations of the base class themselves get called, instead of your overridden ones.

This is NOT a bug in your library, but likely an oversight (or questionable design decision) by the developers of the original TwoWire, who probably never thought that their class could be subclassed. Nevertheless, this makes the whole point of deriving your class from TwoWire useless. To make something useful out of it, we would need to ask the virtual qualifier to be added to all the relevant methods in the original Wire library.

I think we can try to do this, what's your opinion? This would open a lot of other interesting possibilities, even for other libraries :).

[SukkoPera]

Sorry, just noticed this has also been brought up in #27.

[bperrybap]

I would argue that deriving from the TwoWire class in Wire.h doesn't solve the problem that it seems to be trying solved and creates some new problems.

My issue with inheriting TwoWire from <Wire.h> is that beyond the inheritance issues, it creates new issues, like breaking existing code and the documentation of that existing code.
SoftwareWire is now dependent on other Wire libraries from arduino.cc as well as 3rd party cores. Also keep in mind that since <Wire.h> is being used, that the Wire library will be compiled and linked in by the IDE even if it is never used. The twi code cannot be removed from the linked image by the linker due to it using ISR vectors and not being creative with weak symbols. Just including <Wire.h> will increase code and RAM size even if none of the Wire or TWI code ever called or used.

So overall, IMO, it is not a good idea.

It breaks existing code (like my hd44780 library) that has the ability to use the SoftwareWire library since a prior way of using the SoftwareWire library no longer compiles much less work.
i.e. a sketch can no longer create its own Wire/TwoWire object named Wire.
Yes SoftwareWire now creates a Wire object for you, but you can no longer create a Wire object with alternate pins.

In terms of converting everything in the API to use virtual functions, while using virtual functions can look appealing from a development & maintenance perspective, the problem with making everything virtual methods/functions is that it will make the code bigger and slower, will use more precious RAM, and if that isn't bad enough, ALL virtual functions will be linked into the image including methods/functions that are never used. (virtual functions cannot be optimized out by the compiler/linker the same way that regular functions can)
So you have be careful when using virtual functions when you really care about resources and speed.
For example, with IDE 1.8.10 compiling Small_Example for the Uno.
With 1.5.0 the code is 2532 and ram is 69 bytes
With 1.5.1 the code is 3424 and the ram is 206 bytes.

Trying to make things transparently "just work", like the Wire library is an impossible problem to solve.

In terms of helping out some 3rd party libraries that depend on a Wire object, it might be able to do that in some cases; however, it breaks other usage cases that used to work.
Not only that but by using and depending on the Wire library TwoWire class, the SoftwareWire library is no longer it its own entity and so there can be other issues like potentially breaking when arduino.cc releases a new Wire library or a 3rd party core that has its own Wire library makes a change.

For example prior to the 1.5.1 release it was possible to use SoftwareWire and have the sketch create the Wire object using whatever pins the user wanted to use.
like this:

#include <SoftwareWire.h>
SoftwareWire Wire(sda, scl); // create Wire object with desired pins

This was nice since it was all self contained in the sketch, allowed the user to configure any sda, an scl pins he wanted, and could provide a global Wire object for any/all other libraries that needed a Wire object assuming that other libraries used by the sketch didn't include <Wire.h> them.
While this works for my hd44780 library, it doesn't work for other libraries that included <Wire.h> which is nearly all of them.

Overall, there is no way to fully solve the problem of sketches and libraries that want to use <Wire.h> and the Wire object, since the issues are all outside of SoftwareWire, in the Wire library and the 3rd party libraries that have been hardcoded to use the Wire library
Because of this, it cannot be solved in SoftwareWire.

Now you could play some very ugly games and cheat by knowing how the Arduino IDE handles its libraries and include paths.
This would be taking advantage of the stupidity of how the IDE handles its "libraries".

SoftwareWire could provide its own Wire.h header file inside the SoftwareWire library.
And while the IDE now attempts to prefer a library in a directory with the same name as the header file, the way it does this, doesn't really work and the SoftwareWire library could take advantage of this.

The IDE creates an include path for library headers based on the libraries it "sees" in the sketch and then other libraries it "sees" from those libraries.
The order is based on the order it "sees" them.
If a sketch includes <SoftwareWire.h> before <Wire.h> (or if <Wire.h> is not included) the SoftwareWire library directory will be on the include path before the directory of the "preferred" Wire library that contains the Wire.h header file for the selected Wire library. (remember that there can and will be multiple Wire libraries if multiple cores are installed)
This means that any other code or libraries that do include <Wire.h> would get the Wire.h header file from the SoftwareWire library rather than the one from the selected Wire library because the SoftwareWire library will be on the include path ahead of the directory for the chosen Wire library.

Yes it is VERY ugly, but the SoftwareWire library could take advantage of this to make things work better and no longer by tied an external TwoWire class.
Hosever, even in doing all this ugliness, the Wire library and twi code will still be compiled and will be linked in.
There is no way around this since the IDE will do it based on seeing <Wire.h> being included by the 3rd party library.

The real answer, is that libraries that depend on a "Wire" library API need to use templates so they can be passed the name of the "Wire" object and its class and morph accordingly.

Eventually, I'll be moving that way with my hd44780 library to escape from all this Wire library craziness.
However, it looks like this wouldn't work with SoftwareWire given the inheritance virtual function issues with the TwoWire class in the Wire.h header file.

[SukkoPera]

I agree on some of your observations, but not all of them.

If we really want to solve this issue once for all, we have to come up with a meaningful proposal to the Arduino team, one that balances resource usage, ease of customization and backwards compatibility.

Using templates has the disadvantage that it does not enforce uniformity of interfaces i.e.: different implementations might have slightly different methods (beginTrasmission() returning boolean or int, for instance) which might make them not 100% compatible.

I am no expert as to how C++ code gets compiled, but I think I understand why the compiler cannot optimize away virtual methods even if they are not called. But then I think the cleanest solution would be to have a TwoWire class that is fully abstract, i.e. with only pure virtual methods. Then Wire would be a concrete implementation of it, and SoftwareWire would be another one. Interface uniformity would be guaranteed and including the latter would in no way link in anything from the former. I would also expect the compiler being able to do more clever optimizations if it only sees a fully abstract class and a single implementation of it (I'm not actually sure of this, as again I am no compiler guru, but in theory I don't see why it should not).

What do you think?

[bperrybap]

gcc used to have the ability to optimize out unused virtual functions in certain situations.
The code had to do some special (and ugly) things, like patching a vtable entry. It was removed a few versions back, I'm not familiar with the reasoning for removing it but I'm not sure if it will ever come back given things are not typically not removed unless there is consensus for it.
Optimizing out unused functions is complicated, it isn't done by the compiler, it is done though a linker pass at the end, by using some information in the object files provided by the compiler.
For virtual functions, it can start to become impossible since some of the linking is done at run time and so the linker doesn't have enough knowledge at link time to handle many situations so it has to link in the code "just in case" it ends up being used since it can't tell for sure it isn't being used at link time.

On the technical side of things of enhancing the Wire library, it is much more than just convincing Arduino.cc to make a change (see more on that below), It requires all the 3rd party core vendors to buy into it as well. At best, even if Arduino.cc went along with it, it would likely be years, if ever, before it propagated around to all the cores and they stopped shipping their own full/self-contained version of the Wire library.
And why would they? What would be their incentive at this point?
Arduino.cc has a history of breaking things by doing dumb things that some of these core writers may still remember as they may have been burned by them in the past.
Just look at the Arduino 1.0 debacle. The Arduino team knowingly and intentionally decided to break 100 % of all existing 3rd party code with the Arduino 1.0 release, even when there were specific proposals offered that were trivial (like a few lines of code) to implement that would have eliminated over 99% of the incompatibility issues. (The Teensy core went ahead and implemented them and provided them with the Teensy core)
That was in Nov 2011 (about 8.5 years ago). It was a 2 rough years of fixing code by the 3rd party community to recover from this, and we still occasionally see issues in code (old code) out there do to this.
Then there are 3rd party Wire libraries that have extensions, fixes, or have written their own better version of the code for speed or other reasons. Some (like pic32/ChipKit) do not use the same TwoWire class name or even used the same class structure.
Or in some cases they may already have multi bus support, They are not going to want to switchover to what Arduino.cc comes up with as it would likely mean losing some capabilities that they currently have.

And then there may also be implications for 3rd party libraries that use the Wire library. They may also have to make changes depending on the level of backward compatibility offered by a new Wire library and Wire API.
They might also have to use conditionals to be able to support both the current Wire library and the new and improved Wire library.

Getting everyone to switch over to something new and fully depend on Arduino.cc for a new version of Wire library & Wire API is a big deal at this point. IMO, the only way that something like this might work would be come up with a new i2c library rather than try to change/update/migrate the existing Wire library.
By creating a new library, it could also address and solve issues like multiple i2c buses.

And even then, like I said, using virtual functions is nice for creating an internal layered API, but there is a cost.
For environments like the AVR which can be extremely resource limited, like in the TINY series, using up extra code and RAM memory is a big deal.

I know I sound pretty negative, and I could be mistaken, but I'm assuming that there will be quite a bit of resistance to adopt a new Wire library design by the 3rd party developers, particularly for low resource environments like the AVR. Especially since for the most part, it would be a lot of work to switch over, and there wouldn't be much, if any, benefit to them, but there would be some downsides.

---

In terms of getting the Arduino team to change something, that is a very difficult thing to do. I've had a few successes doing this but it is pretty difficult, very time consuming, and most of my attempts and attempts I've seen by others have failed, including when patches/pull requests have been offered and the update was transparent and fully backward compatible with all existing code.
Here are few of the ones I remember that have failed over the past 10 years:

• better way of handling of how Arduino "libraries" are compiled and linked in to avoid library collisions
  (currently this is still mess and the IDE can stomp on a users personal libraries in his sketchbook)
• support for printf() in the Print class
  (many 3rd party cores already support this)
• speeding up digitalRead()/digitalWrite() for AVR core by 40-50X
  (full patches were provided and it was totally transparent and backwards compatible)
• fixing Print class so write(0) works.
  (full patches were provided and it was totally transparent and backwards compatible)
• adding an option to enable floating point to AVR core printf()
  (several patches were provided for this, including one for a GUI option in the IDE)

There are many more.

[bperrybap]

I have decided that for my hd44780 library, I will advice my users to avoid the 1.5.1 library version and to stick with version 1.5.0 as 1.5.1 is not only larger due to it dragging in the Wire library code, even if not used, but version 1.5.1 is not compatible with the way the hd44780 library was using it due to the inclusion of the <Wire.h> header file.

On a side note, IMO, from a SemVer version numbering perspective, the change to inheriting TwoWire is not a patch number release bump, it is a major version number since it has changed things in a way that is not backward compatible. i.e. it should have bumped to version 2.0.0

[Testato]

mmm
so if it is useless, untill Arduino team decide on Virtual class question, we may return officially to 1.5.0 ?

[bperrybap]

In my opinion, I don't see the value of inheriting TwoWire, but it isn't my library.
I would encourage thinking about what it solves for the library user vs what issues it creates.
Things like, Does the change offer a benefit the user vs just making things a bit different under the hood. etc...

In terms of going back to 1.5.0, it looks like a few fixes were done since the switch to inheriting TwoWire, so if you do decide to revert back to pre TwoWire inheritance, those updates would need to be pulled back in.
Perhaps the inheritance code could be moved over to a branch for further testing / development?
but not yet part of the tagged Arduino releases?

I will say if you do decide to back out 1.5.1,
be very careful about git version tags and make sure that the .properties file ALWAYS matches the github tag. I've seen another library that created a release with a conflict and it hopelessly confused the library manager and they haven't figured out how to resolve it since some of this version and tag information is cached on by the Arduino library manager.
i.e. jump to a higher version and don't try to remove 1.5.1 or re-tag some future version as 1.5.1

[bxparks]

FYI, this PR (arduino/ArduinoCore-avr#396) attempted to convert about 21 TwoWire methods to virtual, but was reverted, because I pointed out that the new code increases flash memory consumption by ~650 bytes on AVR processors.

Reading over the comments in this issue, I basically agree with everything @bperrybap has written. There is little advantage of inheriting from TwoWire since child classes of TwoWire cannot be used polymorphically. I mentioned in arduino/ArduinoCore-avr#396 that it is possible to use compile-time polymorphism using C++ template to select either TwoWire or SoftwareWire at compile-time, exactly as @bperrybap suggested above. That seems to work, because the method signatures are compatible.

BTW, SoftwareWire has a virtual destructor, but I think it should be removed. Because none of its parent classes (TwoWire, Stream, Print) provide a virtual destructor. So in the following code, the delete calls the wrong destructor:

TwoWire* myWire = new SoftwareWire();
delete myWire; // calls ~TwoWire::TwoWire(), NOT ~SoftwareWire::SoftwareWire()

So the virtual destructor does not help at all. But it does increase flash memory consumption by about 500-600 bytes on AVR processors, because the virtual destructor pulls in free() and malloc() even if the destructor is never called.