swapped HardwareSerial for Stream in Firmata.cpp

[soundanalogous]

I realized that Firmata.cpp should include Stream.h rather than HardwareSerial.h since any type of Stream may be used with Firmata. I've updated the examples to include HardwareSerial.h before Firmata.h.

This change is going to break pretty much every existing sketch that uses Firmata with Serial but I think it's a worthwhile change going forward.

Thoughts?

[DomAmato]

can you explain more how this breaks everything?

[soundanalogous]

Because by removing HardwareSerial.h from Firmata.cpp, any Arduino sketch that includes Firmata.h and uses Serial as the transport will not compile. HardwareSerial.h would need to be included in all of those sketches. RobustFirmata for example would not compile until you include HardwareSerial.h

[rwaldron]

IIUC, the breakage is trivial to fix: just add HardwareSerial.h?

[soundanalogous]

It's a simple change for sure, but my biggest concern is that it is not an obvious change. There are many people who have their own sketches that depend on the Firmata library. Even Adafruit, Sparkfun, RedBear labs and other companies bundle their own version of StandardFirmata with some of their libraries (mainly for various BLE boards). Some of them have a fork of the Firmata lib itself so their sketches would not break. However anyone else who opens a sketch after updating the Arduino IDE to a version that includes this change, will get an error while compiling and will have to know to add HardwareSerial.h to their sketch to fix it. Maybe I'm just over thinking things. I have a hard time making the call to make potentially breaking changes.

[rwaldron]

I have a hard time making the call to make potentially breaking changes.

I'm no stranger. I'm going to leave an inline comment for you momentarily.

[rwaldron]

So... I was going to suggest this: "What if you add some comment text here that simply says something like: "If you encounter a compilation exception error due to..." BUT then I realized they won't see this file open in the Arduino IDE and that is the crux of the problem. I think mitigation is insuperable. Does the Arduino IDE's new library system have any sort of "changelog" mechanism? Should it? (I suspect maybe?)

[zfields]

I feel fairly confident this won't cause a break on any of the Arduino sample sketches for a couple of reasons...

• Mainly, because Arduino.h includes "HardwareSerial.h" directly.

...
229 #ifdef __cplusplus  
230 #include "WCharacter.h"  
231 #include "WString.h"  
232 #include "HardwareSerial.h"  
233 #include "USBAPI.h"  
234 #if defined(HAVE_HWSERIAL0) && defined(HAVE_CDCSERIAL)  
235 #error "Targets with both UART0 and CDC serial not supported"  
...

• The FirmataClass::begin signature has a Stream type argument, but you aren't including HardwareSerial.h or Stream.h in Firmata.h, therefore Stream would be undefined if Stream.h weren't already included in some way or another.

However, it is good practice to include the headers that you need, so I am not suggesting that you remove it altogether. If it were me, I would move the inclusion of Stream.h into Firmata.h

[soundanalogous]

I was just thinking the exact same thing. I thought back when I first submitted this that I had tested and ran into compiler issues but my memory is hazy. I'll test this again sometime soon.