Chris--A / PrintEx

An extension to the Arduino Print library, and much, much more...
GNU General Public License v3.0
61 stars 16 forks source link

extend base class rather than create parallel PrintEx class #13

Open bperrybap opened 8 years ago

bperrybap commented 8 years ago

Wouldn't it be possible to create a templated PrintEx class that extends the base class with the new functions rather than creating a parallel class? So that the printf() function is added to the object vs creating an additional parallel object. i.e. it would be really nice not to have to deal with two different objects. (1 for printf() printing and the original for everything else)

The constructor initialization might look a little funky as you would have to pass in the base class in to the PrintEx template constructor which would wrap the orignal base class.

Chris--A commented 8 years ago

Wouldn't it be possible to create a templated PrintEx class that extends the base class with the new functions rather than creating a parallel class?

Just to see if I'm on the right track, Is this what you are asking:

Currently the lib is used like this:

PrintEx printer = Serial;

//...
Serial.begin(9600);
printer.printf();

But you are proposing a change which would allow this?

PrintEx printer = Serial;

printer .begin(9600);
printer .printf();

If this is what you are after, great idea! I have wanted to do something similar, I just haven't put the time in to attempt it.


I have had an investigation and come up with a method. It appears to work well, however it will need a little testing. I'm happy to add it, but is this what you are ultimately looking for:

Working code with changes:

#include <PrintEx.h>

auto &printer = PrintEx::Wrap(Serial);

void setup() {

  printer.begin(9600);
  printer.printf( "%20n\nFirst printf use!\n%20n\n\n", '=', '=' );
  printer.println("foobar");
}

void loop() {}

It uses C++11 auto but can also work in C++98, just have to write out the complex type. I think I'd only add this in the documentation as a C++11 feature, I just wont mention the C++98 version (its ugly).

The above sketch outputs:

==================== First printf use! ====================
foobar

bperrybap commented 8 years ago

Yes that looks like what I was thinking. It offers the ability to create an object that includes all the original methods and contains all the PrintEx methods. This looks like a big win in that the application then deals with a single object.

Rather than depend on a recent C++ capability and create a new object, I was wondering if it could be done with a template so that the templated class creates the wrapper class, and you actually end up with a single object that is the wrapper class that inherits the class of both PrintEx and some other class that inherits the Print class.

So you pass in the name of the "base"/original class to the template when the constructor is called and use that to create the new wrapper class that inherits the orignal class as well as PrintEx.

The intent being that there is one templated constructor that creates one object but is told the name of the "base" class so it works with any class that inherits from Print.

I was going to look into trying to do this, but I haven't gotten around to it yet.

Chris--A commented 8 years ago

Rather than depend on a recent C++ capability and create a new object, I was wondering if it could be done with a template so that the templated class creates the wrapper class

If I understand you correctly, this is what it is doing. The wrapper class can be typed in for C++98. The auto keyword in C++11 simply makes this easier.

PrintExWrap<HardwareSerial> &printer = PrintEx::Wrap(Serial);   //C++98

auto &printer = PrintEx::Wrap(Serial);    // C++11
bperrybap commented 8 years ago

I guess I was thinking of the case where the "base" class requires a constructor like say LiquidCrystal. I just haven't thought through how to handle it yet to see if there was a way to do without creating two objects.

Chris--A commented 8 years ago

@bperrybap

I have made a few additions and the sketch below compiled (Do not have a setup to test it). However, so far it is looking good!

This was done using a LiquidCrystal example sketch.

#include <LiquidCrystal.h>
#include <PrintEx.h>

PrintExWrap<LiquidCrystal> lcd(12, 11, 5, 4, 3, 2);

void setup() {
  // set up the LCD's number of columns and rows:
  lcd.begin(16, 2);
  // Print a message to the LCD.
  lcd.printf( "%5n Test! %5n", '=', '=' );
}

void loop() {
  // Turn off the blinking cursor:
  lcd.noBlink();
  delay(3000);
  // Turn on the blinking cursor:
  lcd.blink();
  delay(3000);
}

So now PrintEx::Wrap(Obj); can wrap an already existing object (create a reference). Or you can go ahead and declare an instance of a PrintExWrap<T> to create a new instance.

bperrybap commented 8 years ago

That looks great and is exactly what I was looking for. I'll be happy to do some testing of it. If you are not ready to fully commit it, just put it on a branch and I'll clone the repo and do some testing.

I'm looking at providing example sketches that use this with my openGLCD library and my about to be released hd44780 library.

One area that I want to test will be the Teensy environment. Paul supplies a printf() function in his Teensy core Print class . I'm assuming that thePrintEx printf() will override that when used, which is to be expected.

I'm surprised you are releasing this under such a liberal license. MIT license is basically freeware. My view is that code developed through hard work should not be offered for free to those who won't share their code, especially in cases where there are no other readily available alternatives. In my view closed source compatible licenses essentially make the developers unpaid employees for commercial entities using the code in their closed source products.

There are a few other minor functional tweaks that would be nice but those are off topic from this issue and I'll create separate issues for them.

--- bill

Chris--A commented 8 years ago

Cheers for the feedback.

I'll add in my changes today I'll mark the change as a beta option and may be subject to tweaks and name changes. I have a few bugs I've discovered and am currently testing their fix. Once added I'll create an issue pointing out the parts of the code that need verifying.

I've been contemplating changing the licence to the same as Arduino's. I guess it is alright to change a licence as people who have already downloaded a previous version cannot have their licence revoked. And will only affect new downloads, or version upgrades.

Chris--A commented 8 years ago

This has now been added. I have a few more things to include then I'll push a new tag so it'll show up in the library manager.

Chris--A commented 8 years ago

@bperrybap

I'm pretty happy with this, although I'll reopen this issue until we can test it in a variety of circumstances.

One feature I forgot about still works which is great. This is using StreamEx or PrintEx as a function parameter. So generic printing and reading functions still work fine:

#include<PrintEx.h>

void setup() {
  Serial.begin(9600);

  auto &printer = PrintEx::wrap(Serial);

  func( printer );
  func( Serial );  
}

void loop() {}

void func( PrintEx p ){
  p << "Hi!" << ios::endl;
}

My major concern is the constructors used to pass through initialization. I haven't found any problems yet, but I'm sure there is a circumstance where they might fail. I'll have to do a deeper investigation.

bperrybap commented 8 years ago

I cloned the latest code. overall, really cool. Trying to use this with my hd44780 library with some issues.

This fails: PrintExWrap<hd44780_I2Cexp> lcd; // declare autoconfig lcd object

This works: PrintExWrap<hd44780_I2Cexp> lcd(0x27); // declare lcd object at address 0x27

It seems to fail when there are are no constructor arguments. Also, some constructors use more than 8 parameters. LiquidCrystal class can use up to 11 constructor arguments.

Unfortunately after 35 years of embedded C programming, I'm an expert with cpp macros but sorely lacking with C++ template wizardry so I can't fix what I assume is a simple missing entry for the no argument constructor/definition/declaration case down in PrintExtension.h

These also work: (on newer IDEs, 1.6.6 and newer)

hd44780_I2Cexp _lcd; // declare hd44780_I2Cexp lcd object; auto config
PrintExWrap<hd44780_I2Cexp> &lcd = PrintEx::wrap(_lcd);
hd44780_I2Cexp _lcd; // declare hd44780_I2Cexp lcd object; auto config
auto &lcd = PrintEx::wrap(_lcd);

Even the non auto version fails on IDEs older than 1.6.6 There are some C++11 issues in TypeTraits.h that use constexpr

This currently limits the library use to only a limited number of very recent IDEs since some IDEs have serious issues and have to be avoided. 1.6.7, and1.6.9 are the only IDE versions that currently can be used due to serious issues in 1.6.6 and 1.6.8

This probably should be another issue: Have you considered supporting older IDE versions that don't support the 1.5x library structure? I avoid using the src directory so the library can still work with older IDE versions.

Chris--A commented 8 years ago

I have changed the code to compile for C++98. I'll add more constructors or look at adding a macro to generate a list covering even the longest constructor.

Also where is the library you were testing with, I can only find https://github.com/duinoWitchery/hd44780, google says there is this repo: https://github.com/duinoWitchery/hd44780_I2Cexp/ however, it just 404's

It seems to fail when there are are no constructor arguments.

I've also added in a default constructor, was this a compile error, or just not working? I'd have expected default construction to take place as this is should be implicitly generated.

bperrybap commented 8 years ago

Yeah the duinoWitchery organization is mine. It is the new location for all my Arduino related "stuff", including this new hd44780 library. I'm in the process of a major refactoring of my hd44780 library and haven't yet pushed up anything. There used to be multiple hd44780 sublibraries (one for each i/o subclass) and now everything and all the i/o subclasses are integrated into a single hd44780 library. As part of the re-factoring, the i/o subclass git repositories were removed as they are no longer needed. I haven't done a push of the new refactored code yet as I'm not quite ready for it to be out in the wild yet. It still needs a little bit of cleanup and polishing.

It should up on github fairly soon. (not hours, but hopefully a few days....)

In terms of the template issue. It is a compile error. It seem that the template is not expecting a definition of an object with no constructor arguments.

You can see the same error by inserting this into a module:

#include <Wire.h>
PrintExWrap<TwoWire> MyWire;

That will attempt to create a TwoWire object that defines the object with no constructor parameters. The resulting error is the same as what happens with my hd44780_I2Cexp class.

bperrybap commented 8 years ago

Hey, I just pulled the latest updates in and the template issue has been resolved! It now all works with my library. All 3 ways of doing wraps and the non "auto" definitions work on the older non C++11 IDEs. Very cool.

I'll have do some testing with Teensy. That one will be interesting as Paul's Teensyduino Print class already contains a printf() function. I'm assuming that when using the wrap() it overrides that one.

bperrybap commented 8 years ago

Just did a quick test on Teensy. It works as expected. When using the PrintExWrap the printf() overrides the printf() built into the Teensy Print class.

Also tested with the ChipKit pic32 core on an UNO32 board also worked as expected.

Seems to be working as expected so far. I REALLY like it.

Chris--A commented 8 years ago

Cool,

Cheers for testing. I'm still checking through things, but I'll update the docs and version today hopefully (I might hold off on the release though). I'm also looking into legacy IDE support, I'll create another issue for that as there is a few different options.

Also tested with the ChipKit pic32 core on an UNO32 board also worked as expected.

Nice, I'll have to add all the boards you used to my list.

Just did a quick test on Teensy. It works as expected. When using the PrintExWrap the printf() overrides the printf() built into the Teensy Print class.

That is good to know. I've got some improvements I'm working on that might change the internal declarations a bit. I'll have to set it up on another branch to test it accordingly.

bperrybap commented 8 years ago

I test my libraries (openGLCD & hd44780) on all the h/w I have: Uno, Mega, Leonardo, SleepingBeauty (AVR 1284 board), Teensy 2, Teensy2++, Teensy 3.0, Teensy 3.1, Teensy LC, Teensy 3.5, and ChipKit UNO32.

Across more than a dozen IDE versions. It is a pretty extensive test cycle.

I'll be adding PrintEx examples to openGLCD and hd44780 to show users how to use the extensions and the streaming capabilities and it will be tested on all those boards.

My libraries work on most IDEs since 1.0, openGLCD works on pre 1.0 IDEs as well. (working on pre 1.0 is painful in some cases, since in some cases you have to re-create missing features like the F() macro if you want consistency) I would think there might be some very difficult issues for your library. I decided not to support pre 1.0 on hd44780. There are a few total crap IDE releases that are simply too broken to ever support that I recommend avoiding. I'll look for your other issue related to older IDE support.

bperrybap commented 8 years ago

Here is an accidental oddity:

I accidentally compiled using the Yun board type and the wrap for the Serial object failed:

This does not work: PrintExWrap<HardwareSerial> &__Serial = PrintEx::wrap(Serial);

Here is the output from the IDE:

PrintEx:29: error: invalid initialization of reference of type 'PrintExWrap<HardwareSerial>&' from expression of type 'PrintExWrap<Serial_>'
 PrintExWrap<HardwareSerial> &__Serial = PrintEx::wrap(Serial);
exit status 1
invalid initialization of reference of type 'PrintExWrap<HardwareSerial>&' from expression of type 'PrintExWrap<Serial_>'

This was using IDE 1.6.9

It turns out that on any of the 32u4 based boards, leonardo, yun, etc.. The Serial object class is not HardwareSerial it is actually Serial_

So the safest way to do a wrap is to use the type of original object like this: PrintExWrap<typeof(Serial)> &__Serial = PrintEx::wrap(Serial);

This is a very important useful tip. So if/when the new wrap features are documented or some examples are created, I recommend using the typeof() rather than the actual class name so that it always works. It also is easier.

The reason I was playing with the Serial object is I wanted to create and ehanced Serial object by using wrapping and not have to use a new object so that there are not two objects to deal with. Perhaps there is a better way but I was doing it with the help of some cpp magic/ugliness:

PrintExWrap<typeof(Serial)> &__Serial = PrintEx::wrap(Serial);
#define Serial __Serial

So that at least from an application source code perspective there is only one object.

Chris--A commented 8 years ago

typeof is a GCC extension for C++11's decltype. So on Arduino you can interchange them.

In IDE 1.6.6 and above, you can just use auto &__Serial = PrintEx::wrap(Serial); which is probably the easiest.

I could always add a macro helper to make non C++11 code a bit nicer.