Testato / SoftwareWire

Creates a software I2C/TWI bus on every pins
GNU General Public License v3.0
148 stars 33 forks source link

printStatus() should Print class instead of HardwareSerial class #8

Closed bperrybap closed 6 years ago

bperrybap commented 7 years ago

Currently the printStatus() routine takes an argument of HardwareSerial. This is used to point the routine to the proper output device which is assumed to be a serial port. The issue is that some Attiny cores do not have a hardware serial port and hence no HardwareSerial class. And while the function printStatus() is not really used and will be removed by the linker, the code will not compile in environments that do not have a HardwarSerial class. The simple solution is to change printStatus() to ue an argument of type Print instead of HardwareSerial. C++ will handle the referenced object appropriately since HardwareSerial inherits Print. This is appropriate because the code in printStatus() is only using methods from the Print class. It does not need access to any of the HardwareSerial class methods.

Koepel commented 7 years ago

Normally I would remove that function, but it was very useful to me, so I kept it in the library. I was not aware that it didn't compile for some hardware. I have used a #ifdef to get rid of the function.

About portability: The library allows to use a common single SCL and many SDA for many I2C buses (at the moment with a buffer for every I2C bus, perhaps a single buffer is enough for that situation). It is possible to use it together with the Arduino Wire library. Therefor I would like to keep the 'Wire' name for the Arduino library.

bperrybap commented 7 years ago

There is no need for ifdefs to resolve the HardwareSerial issue and that is not what I was suggesting. (Note: you also have to fix the prototype in SoftwareWire.h) What I was suggesting that you use the Print class instead of the HardwareSerial class for the function argument. printStatus() does not need access to HardwareSerial; it only needs access to Print class methods. That way you pass in a pointer to a Print class object rather than a specific serial type class. There is no need to be so specific. All you have to do is declare your functions and prototypes accordingly. i.e. change them to: SoftwareWire.h

 void printStatus(Print& Ser); // print information using specified object class

SoftwareWire.cpp

void SoftwareWire::printStatus( Print& Ser)

No conditionals with ifdefs and no other code is required to change and no code that calls the function will have to change either. C++ is smart enough to pass in a pointer to the portion of the object for a sub class within an object for an inherited class. i.e. if you can still call printStatus() with a HardwareSerial object and it will work just like it did before since printStatus() was only using Print class methods. printSatus() does not need access to the full HardwareSerial object. The advantage of using Print over HardwareSerial is now it will work with any object of any class that inherits the Print class. So printStatus() would no longer be limited to working with HardwareSerial objects and could also work with other library classes such as SoftSerial, an LCD, GLCD, a network interface or whatever as long as the object uses a class that inherits Print.

note: I might change the variable name from "ser" to "outdev" just for clarity to indicate that it isn't limited to just a serial port. But is it obviously isn't required.

bperrybap commented 7 years ago

About portability: The library allows to use a common single SCL and many SDA for many I2C buses (at the moment with a buffer for every I2C bus, perhaps a single buffer is enough for that situation). It is possible to use it together with the Arduino Wire library. Therefor I would like to keep the 'Wire' name for the Arduino library.

If you are referring to my comment in the other issue over in ATTinyCore, I think you may have misunderstood. I was not referring to any making any changes to SoftwareWire. What I was referring to was sketch code that was wanting to run existing code or libraries that use and depend on a Wire library with a predefined object named Wire. They can simply replace this:

#include <Wire.h>

with this:

#include <SoftwareWire.h>
SoftwareWire Wire(SDA,SCL); // create Wire object (use other pins if desired)

This allows existing code that expects an object called Wire to run without any further modifications. And this is what I have done for some of my testing.

Koepel commented 7 years ago

At this moment I can't test using the Print class. I hope you don't mind that I made a quick fix by disabling the printStatus() in SoftwareWire.h. I will leave this open, and hope to fix it in the future.

An elegant solution for the "Wire" names is hard. I understand that you want to use the same (library) code for everything. However, the ESP8266 Wire library has a few functions of its own. The TinyWire/TinyWireS/TinyWireM have their own little things. And I want to use (one or more) SoftwareWire together with the Arduino Wire library. Using 'Wire' and 'Wire1' and 'Wire2' is the most obvious solution. But then the existing library code has to be rewritten to be able to use either 'Wire' or 'Wire1' or 'Wire2'. That would be good, since new processors have often more than one I2C bus. If you know a good way to write a library that can select the I2C bus that will be used, let me know.

Testato commented 7 years ago

Here "Wire" is only the arbitrary name of a SoftwareWire object constructor.

bperrybap commented 7 years ago

At this moment I can't test using the Print class. I hope you don't mind that I made a quick fix by disabling the printStatus() in SoftwareWire.h. I will leave this open, and hope to fix it in the future. It is pretty simple to test and doesn't require using a library as you can test the technique using sketch code.

It works fine. I've been using this for a long time. It is using a common well defined capability of C++. I even use this same technique in some of the hd44780 library examples to have common code that can print to any Print class device. This allows the same code to print to the LCD regardless of which LCD i/o class is used or to print to a Serial device. In your case using Print would also allow passing in a SoftSerial object instead of a HardwareSerial object. I patched my version of the SofwareSerial code to have the printStatus() method to use the Print class instead of HardwareSerial as it solves the issue and is more flexible.

In term of names to use for names for Wire. It is actually more than just the name of the object, it also can potentially also involve the name of the class used for the object depending on the code and how it uses the object. For library code that uses classes ideal way is to use templated classes so that you can pass in the wire object when the class object is being defined and the class code morphs accordingly and can use the wire object name or class name as needed. This methodology solves all the naming issues.

This would even allow different class objects of the same class to use different "wire" objects. i.e. lcd1 could use Wire and lcd2 could use Wire2 That is where I will eventually be going with the hd44780 library. Most arduino libraries are not set up appropriately to be able to do this.

Another alternative is to cheat and use defines to trick the library using the Wire object into using the proper name by redefining it to something else. hd44780 can currently support this and I've used this methodology for some i2c libraries that declare the wire object for you with some other name than Wire. I can do this in my library because all the code is declared inside the class definition inside the header file. A simple #define in the sketch can be used to rename the name of the Wire object to anything needed. This won't work for other libraries I've seen that use the Wire object as they are putting all the class code in a .cpp file. I went the route of placing the hd44780 i2c i/o class code in the class definition because having all the code inside the class definition is also required if creating a templated class. A side benefit as I mentioned is that you can use #defines inside a sketch to modify library code which can't be done when the code is outside the class definition like it is when it is inside a .cpp file.

Keep in mind that most Arduino users do very simple things, and having a Wire library that "just works" is more important than having the ability to support more than a single i2c bus for the vast majority of Arduino users. My goal is to make an LCD library with examples that "just work" "of the box" for any core or board package that provides a Wire compatible library that predefines its i2c object with the name "Wire" that is already true for most cores and I think that is where things can go with the ATTinyCore board package. Yes the hd44780 library can support alternative environments by editing the examples and playing games with some declarations and defines in the examples, but I feel it is very important to provide a set of examples that "just work" without having to be modified as often as possible and if they don't work for certain environments such as SoftwareWire provide a simple ability for users get something up and running with minimal edits.

For the vast majority of Arduino i2c library code I've seen, it expects a library header named and an object named Wire and it would be a big effort for many Arduino users to change it.

The method I offered in ATTiny issue was a simple way to get the hd44780 library examples using SoftwareWire up and running with minimal changes. It only involved a small change to the sketch with no changes to the hd44780 library code itself. It also didn't require or even suggest any changes to the actual SofwareWire library itself.

Testato commented 7 years ago

For the vast majority of Arduino i2c library code I've seen, it expects a library header named Wire.h and an object named Wire

This is new for me, thanks for the clarification

bperrybap commented 7 years ago

It makes sense if you think about it. A library for a device that uses i2c like a sensor or an LCD has to be written to some sort of API to use i2c. The most common boards out there are Arduino boards that use m328 or mega and those boards which use the standard Wire library that comes bundled with the IDE. Many other boards like the chipkit boards, DUE and other arm based boards like the Teensy boards, and even the ESP8266 boards all provide an i2c library named Wire that uses a header named Wire.h and a predefined object named Wire to access i2c services. So many library authors that need to use i2c for their device simply hard code their library to use Wire.h and the Wire object. It is simple and provides a broad coverage across quite a few platforms. The issues start to arise when library users want or need to use some other i2c library because they may want to use a different i2c bus in the case of multiple buses, or in the case of wanting to use a s/w i2c library. Some libraries have conditionals that look at defines from header files to try to detect if another i2c library is being used but most do not. And the ones that do this are limited to the libraries that they have specific checks for. Like mentioned earlier one way to solve that is to create a device library class that uses a template. This way sketch can pass in the i2c device object and the library code can 'magically' adjust to the name and type of the i2c object being used. But very few libraries use templating as it is a more advanced way of writing/implementing the class code.

bperrybap commented 7 years ago

Another reason to use Print instead of HardwareSerial is that the default Serial object used in a core is not necessarily HardwareSerial. Also, some cores do not use a hardware uart by default so if you are using one of those cores using HardwareSerial in printStatus() would not work, but using Print would. i.e. think about boards that use a virtual comm interface over native USB for their serial port on chips like the AVR atmega32u4 . While they may have a serial object named Serial it isn't using a class named HardwareSerial. And some like the Teensy boards have multiple serial interfaces, a virtual serial port over native USB which does not use HardwareSerial and a UART serial interface that uses the HardwareSerial class.

Koepel commented 7 years ago

Your point about the the HardwareSerial and printStatus() is clear.

A template for 'Wire' when a (sensor or display) library uses I2C would be the best solution. Thanks. Now you only have to convince everyone else.

bperrybap commented 7 years ago

@Koepel I have no clue what you are talking about. "convince everyone else" of what? So far I have not mentioned or even suggested any changes to SoftwareWire or Wire. (other than the change to printStatus() in SoftwareWire) Or suggested using C++ templates in either of them. Nor have I suggested that any other author of an Arduino library that uses i2c services make any changes to their library. Every author of an Arduino library that uses i2c services is free to use their own design and to make their own implementation decisions. Remember I'm talking about libraries that use ic2 services, not sketches. Sketches can do anything they want without concern for compatibility or portability. Libraries that need to use other libraries for services like i2c do not have that luxury, especially if trying to provide examples that "just work" out of the box without any editing. All I stated was that an Arduino library that uses i2c services can work with any Wire API compatible library with an object of any name and any class name by using a templated class in their library. This requires no modifications to the i2c services library, not to Wire nor to SoftwareWire nor to any other i2c services library. And this is the direction I will be going in my hd44780 library. This will allow hd44780 to work with any Wire API compatible library regardless of the actual class name or object name used by the i2c services/library. So when I add template support to hd44780, the user will be able to specify the i2c services library object name when the hd44780 lcd object is being declared and the compiler mutates/creates the code accordingly as needed for that hd44780 lcd object to work with the specified i2c services class object. i.e. if the name of the object is Wire, you pass that in, if is Wire2, pass that in, etc... In something like hd44770 you could then have multiple lcd objects with one object using an object named Wire that is the Wire library and another lcd object using an object name Wire1 that uses the SoftwareWire library. No problems, easy peasy. THAT is the best way to handle this type of issue and is an example of what C++ templates is great at handling. And that is where I'm headed by moving to using templated classes in hd44780. Other library authors of libraries that use i2c services are free to make their own design decisions. However, I don't think that there is any other other way to offer this capability in a library in a portable manor without using C++ templates.

And honestly adding in the support for templates is not high on my list of priorities since I already have hd44780 and the i2c examples working "out of the box" on pretty much every single Arduino core other than the attiny cores. But I believe that will soon be fixed in the ATTinyCore core as it will soon include a Wire compatible library that creates an object named Wire by default. And for environments that want to use SoftwareWire with hd44780, one way to get the existing hd44780 code to work without needing template support is if the sketch simply names its SoftwareWire object Wire. (which is what I mentioned in the other issue over in ATTinyCore) There are other ways to get it work if the object name is other than Wire, without having to edit library code. I was just providing a quick and easy way of doing it for testing.

The templates in hd44780 are useful for more complex environments that want to use multiple i2c buses. Which for the most part are not very common and that is why it is not very high on my list of priorities.

Koepel commented 7 years ago

I was thinking about the future of libraries (for sensors and display) that use I2C. Since newer processors can have more than one I2C bus and the SoftwareWire could have Wire1...Wire10, and all the other 'Wire' compatible libraries and even using a few different ones at the same time... then I a template could be the solution.

You have to read the sentence "Now you only have to convince everyone else" with a blinking eye emoticon at the end.

Testato commented 7 years ago

@bperrybap May you made a PR for the HardwareSerial modification ? so we can accept it and close this issue. Thanks also for the Wire object name discussion, it been very interesting, for now i never used C++ template programming, and like you said it is not strictly necessary to SoftwareWire use it for now, because you can always use an object named "Wire".

bperrybap commented 7 years ago

Are you asking/wanting me to create a pull request for the change from using HardwareSerial to Print class? Its a very small change, but if you want me to I'll do it.

Even if templates are used in a library that uses SoftwareWire, there would be no changes to SoftwareWire.

Testato commented 7 years ago

yes, but only for have you officially in the contributor list :-) you can do it also directly online, github is very powerful on this kind of little PR creation, you will done it in 10seconds. But if you prefer i can do it for you. Thanks again for you support