Closed bxparks closed 3 years ago
For me it's ok, What do you think ? @Koepel @bperrybap @sukkopera
Sounds good to me. As I said in #28, as the TwoWire class does not have virtual methods, deriving from it is totally pointless. I would have preferred "the other solution" (i.e.: Making the methods virtual in the parent class), but since that is not going to happen, the only other meaningful thing to do is this one, as we are currently wasting flash and RAM for no useful purpose.
~When this is merged in, #28 can be closed.~ I see it's already been linked.
It sounds good, but I need to test it with the hd44780 library to make sure it actually works as expected.
The TwoWire and Wire.h were used to be compatible with other libraries that use them. As a result a unused "Wire" object is sitting in memory.
This is related to: https://github.com/Testato/SoftwareWire/issues/28 were @bperrybap already wrote about it.
The goal is to be able to use both Wire (hardware I2C bus) with multiple SoftwareWire ojects in the same sketch and be compatible with all the sensor libraries. Most libraries use pointers to TwoWire, others might use a template. Since more and more libraries store a TwoWire pointer in the object, that should still be supported.
I have even seen Wire.print()
, but I don't care if the Print class is not supported.
The virtual destructor is a bug. Changing the other things will result into trouble in one way or the other in my opinion. I'm afraid we will end up with defines or templates to select an appropriate version of the library.
The patch appears to revert code back to the 1.5.0 release with a few minor changes.
I'm not sure the purpose of that last change, particularly the altering of the handling of SDA (setting it low) But the patch effectively rolls back the 1.5.1 changes. I did some limited testing and it compiles and works with my hd44780 speed test example for SoftWire. This new code is a couple of bytes bigger than the 1.5.0 code but that is likely do to the minor changes made.
@Koepel I'm not understanding your comment:
The goal is to be able to use both Wire (hardware I2C bus) with multiple SoftwareWire ojects in the same sketch
and be compatible with all the sensor libraries.
Most libraries use pointers to TwoWire, others might use a template.
Since more and more libraries store a TwoWire pointer in the object,
that should still be supported.
While I think that compatibility goal is desirable, I don't think it is technically possible to do what you have sated.
i.e. be able to use both Wire and SoftwareWire objects simultaneously (multiple buses) in the same sketch and also be compatible with all the sensor libraries.
I don't think all those desires are technically possible.
I have seen quite a few libraries for i2c h/w slaves that just include
Also, I don't think it is possible to have both Wire and Software library classes use the class name "TwoWire" to allow libraries to work that use pointers to TwoWire and be able to use both libraries simultaneously in the same sketch.
I think the only way a library can guarantee to be compatible with various i2c libraries is to use a template with pointers so that it no longer cares about the object name or its class. (The classname can be passed into the template using typeof(WIREOBJENAME) ) So far I've seen very few libraries that use templates as the typical Arduino library puts code in a .cpp file for separate compilation and that isn't how you do it when using templates.
Currently, the hd44780 library does not use templates yet to handle the i2c class and object. It currently depends on the sketch to include that proper i2c library header and then ensure that a Wire object exists. Eventually, I'll switch to templated class so that it will work with anything that provides a "Wire" s/w API.
Let me rephrase that: Most libraries that try to be able to select a Wire library use a pointer to a TwoWire object.
A typical way would be Sensor.begin( int i2cAddress, TwoWire *wire);
Adafruit uses this sometimes: bool begin(uint8_t i2c_addr = SENSOR_ADDR, TwoWire *wire = &Wire);
I think that we can not drop support for the TwoWire pointer way at this point.
Personally, I like templates make the library compatible at source code level, but the TwoWire pointer way has also some advantages if it is consistently used.
You see problems with both a hardware and one or more software I2C buses in the same sketch ? That would be disappointing.
Let me rephrase that: Most libraries that try to be able to select a Wire library use a pointer to a TwoWire object.
A typical way would be Sensor.begin( int i2cAddress, TwoWire *wire);
Adafruit uses this sometimes: bool begin(uint8_t i2c_addr = SENSOR_ADDR, TwoWire *wire = &Wire);
Yes I definitely agree with that.
Maybe I misunderstood you.
What did you mean by:
The goal is to be able to use both Wire (hardware I2C bus) with multiple SoftwareWire ojects in the same sketch and be compatible with all the sensor libraries.
I took that to mean being able to use objects from both Wire and Software at the same time (multiple i2c buses) and using TwoWire as the name of the class for both. Is that what you meant? If so, I don't see how that can technically work.
i.e. in the absence of having a wrapper class named TwoWire provided by the bundled IDE i2c library you can either have the TwoWire class name used by all the i2c libraries (like this this one) and provide a global predefined object named Wire, which provides great compatibility with the Arduino.cc bundled Wire library but also prevents using more than one i2c library at a time
Or you can have unique class names for each i2c library which allows concurrent use but will not be compatible with libraries that hard code the class name to TwoWire or use a hard coded wire object name like Wire.
So I guess I still don't understand what you mean by:
I think that we can not drop support for the TwoWire pointer way at this point.
Are you pushing for making SoftwareWire fully compatible with the Wire library (which means you can't use the Wire library when using the SoftwareWire library) or something else that allows using both at the same time? Given your response so far, I think you are pushing for something else, but I'm not sure what that is and how it can work to preserved 100% compatibility with Wire and also allow both Wire and SoftwareWire to be used at the same time.
BTW, this same issue exists in other libraries like Arduino libraries that sit on top of libraries for hd44780 displays. i.e. there are a number of lcd menu libraries that allow passing in a lcd object pointer but then hardcode the lcd class name to LiquidCrystal so they don't work with any other lcd library.
The only solution that I have seen that works for all cases (multiple objects for using multiple buses including using multiple libraries using different class names) is to use templates where the user passes in the class to the template when the object is declared.
Adafruit may use a TwoWire*
pointer in its code (e.g. the bool begin(uint8_t i2c_addr = SENSOR_ADDR, TwoWire *wire = &Wire);
), but I am not sure that it is relevant to us because TwoWire
cannot be used polymorphically. As discussed in #28, no subclass of TwoWire
can be used though a pointer to TwoWire
, because wire->beginTransmission()
calls TwoWire::beginTransmission
not SoftwareWire::beginTransmission()
.
If the alternative proposal is that SoftwareWire.h
should be a drop-in replacement forWire.h
, by renaming SoftwareWire
to TwoWire
, then I'm pretty sure that would not work. Since SoftwareWire.h
would cause a name collision with any 3rd party library that happens to do a #include <Wire.h>
.
I do think it is beneficial for the SoftwareWire
class to be syntactically (i.e. source code) compatible with the TwoWire
class, because we can use C++ templates to achieve compile-time polymorphism, instead of using runtime polymorphism through dynamic dispatch. But I don't think it buys us much for SoftwareWire
to be a subclass of TwoWire
. I have convinced myself through this PR that it does harm because it pulls in large amount of dead code which is never used and cannot be optimized away by the linker.
Thanks for the explanation.
Suppose someone has a sensor with a library with a TwoWire pointer for the hardware I2C bus and wants to add a software I2C bus with a sensor library that also uses a TwoWire pointer. I understand now that it is not possible.
If someone wants something extraordinary, then we have to tell to change the sensor library.
So we're all finally in agreement?
I will merge this PR and publish a V1.5.2 to the library manager
thank you all
Agreed, but I'd rather make that 1.6.0.
And maybe you can also check that all method signatures match TwoWire's and close #27 as well.
@SukkoPera:
Agree with 1.6.0 versioning.
Regarding #27, different platforms have slightly different TwoWire
APIs, but I guess SoftwareWire works only on AVR so that's the target. But it's not clear to me that having 100% API compatibility with the AVR version of TwoWire
is actually worth the effort.
Added this page to the Wiki: Using a library that needs a TwoWire pointer Feel free to improve it.
@Koepel: That's good information. I have another way of doing this, but it's not quite ready for public consumption. I'll add some info on that wiki when it's ready.
I recently evaluated many of the libraries listed in this wiki page, https://github.com/Testato/SoftwareWire/wiki/Arduino-I2C-libraries. I added a few entries, added some comments, and refactored the page a bit. The biggest change is that I grouped the libraries according to their target architecture.
FYI: I did another substantial update to the Arduino-I2C-libraries wiki page. Three big changes:
1) I added a "RX/TX Buffer Sizes" column and filled it for each 3rd party library on that page. The buffer sizes range from 0 bytes (no buffer) to 256 bytes on the SAMD implementations.
2) I added a separate <Wire.h>
entry for each of the major platforms that I am familiar with (Arduino SAMD, SparkFun SAMD, STM32, ESP8266, ESP32) because each platform has a slightly different implementation compared to the AVR Wire library, with slightly different RX/TX buffer sizes.
3) I added an entry for my AceWire library that I recently finished, but I realized that it is solving a slightly different problem than the one described in the Using a library that needs a TwoWire pointer wiki page created by @Koepel . Specifically, AceWire provides a thin adapter layer that allows compile-time polymorphism for the application code so that different I2C implementations can be easily selected with little to no code changes at the application layer. That's a different problem than adapting an existing library that is hardcoded to use <Wire.h>
for use with a different I2C library. So left the Using a library that needs a TwoWire pointer wiki page alone.
According to my testing, this library currently consumes about 3926 bytes of flash and 216 bytes of static ram on an Arduino Nano (after subtracting away an empty program that does nothing, i.e. empty
setup()
and emptyloop()
). This PR reduces the flash consumption by 2036 bytes, and ram consumption by 157 bytes, through the following steps:1)) Remove virtual on destructor. Saves 592 bytes of flash and 14 bytes of ram by preventing the virtual destructor from pulling in
malloc()
andfree()
. Making the destructorvirtual
has no benefit for theSoftwareWire
class as far as I can see, because neither of its parent classes (TwoWire
andStream
) defines a virtual destructor. That means thatSoftwareWire
cannot be deleted polymorphically. In other words, in the following code:the
delete
operator calls~TwoWire()
, not~SoftwareWire()
. I have actually tested this, and verified that "wrong" destructor (~TwoWire()
) is called. (It is the correct destructor as far as the C++ language spec is concerned, just not the one that we wanted). When the above code is compiled with warnings enabled, the compiler prints the following warning:which is an additional indication that making the destructor
virtual
does not have the desired effect.2)) Remove inheritance from TwoWire. Saves 304 bytes of flash and 30 bytes of ram. The PR that attempted to make
TwoWire
polymorphic (https://github.com/arduino/ArduinoCore-avr/pull/396) was reverted (https://github.com/arduino/ArduinoCore-avr/pull/412) because it causesTwoWire
to increase its flash memory by about 650 bytes. I think it is safe to assume thatTwoWire
will not support subclassing in the future. So we can save memory by changingSoftwareWire
to no longer inherit fromTwoWire
.3)) Remove including Wire.h. Saves 1140 bytes of flash and 113 bytes of ram. Just the inclusion of the TwoWire header file (through
#include <Wire.h>
) causes over 1kB of flash to be consumed, even if theWire
object is never used. For reasons which are not obvious to me, the compiler is unable to determine thatWire
is never used in the program, and does not optimize it away during link time. Since (2) above changesSoftwareWire
so that it is no longer a subclass ofTwoWire
, we no longer need to include<Wire.h>
.Total flash savings: 2036 bytes, from 3926 bytes down to 1890 bytes Total ram savings: 157 bytes, from 216 bytes down to 59 bytes
This PR causes the following effects:
For people who are interesting in using this patch, I will probably keep around my fork (https://github.com/bxparks/SoftwareWire) while this PR remains unmerged.