Properly implement Stream

[firmata]

Properly implement Stream

[soundanalogous]

Some initial work was done in the stream_fix branch, but is not sufficient.

[ntruchsess]

Wouldn't it be sufficient to just declare FirmataSerial as public member of Firmata?

[soundanalogous]

The stream_fix branch I had created worked fine when using hardware serial, but did not work with softSerial (which implements Stream) so there was something else that needed to be done. I have not looked into it further.

Julian has done some work in his fork. I think he has working implementation that can use Ethernet and Wi-Fi. The solution may exist in his code.

Also it's worth noting that it is likely not possible to support multiple types of connections (ethernet, wi-fi, etc) in StandardFirmata and we may need separate versions for each connection type (as Julian has done except using Stream to abstract the connection). The first step is to update Firmata.cpp / Firmata.h.

[ntruchsess]

I did review Julians FirmataEthernet and wifi-client code and I can't find anything in there that justifies to wrap the Stream used in FirmataClass just to export Streams methods to the XXXFirmata.ino code that cannot access the FirmataSerial member just because it's declared private.
Declaring FirmataSerial as public and use Firmata.FirmataSerial.write(..) instead of Serial.write() would work as well.

[soundanalogous]

Rather than setting FirmataSerial to public, we can add this to Firmata.cpp:

// expose the write method
size_t FirmataClass::write(uint8_t c)
{
  FirmataSerial.write(c);
}

and this to the public interface in Firmata.h:

size_t write(uint8_t);

I think this will work fine and accomplishes the same thing you are talking about.

The issue I was having was that at the end of Firmata.cpp a new instance of FirmataClass is declared passing the hardware Serial FirmataClass Firmata(Serial). This locks you into hardware serial. You are supposed to be able to change the Stream by passing a new stream into Firmata.begin(newStreamInstance) but for some reason I was never able to verify that was working.

Julian gets around this issue by creating a new instance of FirmataClass FirmataClass FirmataEthernet(client) in StandardFirmata passing a reference to the Ethernet stream (however I would think this would create 2 instances of FirmataClass in memory?). If there is no memory issue, then that's a valid solution.

[timschneider]

the reason for lock to hardware serial as stream is, that the stream class has no virtual copy constructor. And as normal in arduino the stream is handled as a reference not as a pointer - so cpp has no chance to copy another serial stream to the compile time generated class.

See this pull request on arduino

arduino/Arduino#119

After this change you are able to beginn(stream& serialstream)

Cheers tim

Jeff Hoefs [email protected] schrieb:

Rather than setting FirmataSerial to public, we can add this to Firmata.cpp:

// expose the write method
size_t FirmataClass::write(uint8_t c)
{
 FirmataSerial.write(c);
}

and this to the public interface in Firmata.h:

size_t write(uint8_t);

I think this will work fine and accomplishes the same thing you are talking about.

The issue I was having was that at the end of Firmata.cpp a new instance of FirmataClass is declared passing the hardware Serial FirmataClass Firmata(Serial). This locks you into hardware serial. You are supposed to be able to change the Stream by passing a new stream into Firmata.begin(newStreamInstance) but for some reason I was never able to verify that was working.

Julian gets around this issue by creating a new instance of FirmataClass FirmataClass FirmataEthernet(client) in StandardFirmata passing a reference to the Ethernet stream (however I would think this would create 2 instances of FirmataClass in memory?). If there is no memory issue, then that's a valid solution.

---

Reply to this email directly or view it on GitHub:
#13 (comment)

[ntruchsess]

in cpp it's illegal to reseat an initialized reference, so there's no way to replace FirmataSerial being a reference to Stream after it's been assigned in the constructor. Declaring FirmataSerial as pointer to Stream works fine though.
Since FirmataSerial is private anyway should be no issues with backwards-compatibility.

[timschneider]

Wouldnt It bei better Titel use Thema virtual copy constructructor?
Sent from Huawei Mobile

Norbert Truchsess [email protected] schrieb:

in cpp it's illegal to reseat an initialized reference, so there's no way to replace FirmataSerial being a reference to Stream after it's been assigned in the constructor. Declaring FirmataSerial as pointer to Stream works fine though.
Since FirmataSerial is private anyway should be no issues with backwards-compatibility.

---

Reply to this email directly or view it on GitHub:
#13 (comment)

[ntruchsess]

I didn't verify whether this virtual copy constructor thing actually works, but I would wan't to rely on a 4 month pull request to arduino core if I allready have a working solution that changes Firmata-code only:

#22

Passing the reference in begin(&Stream) isn't the problem, it's the assignment to the allready initialized reference-member (which you cannot leave unititialized during construction of the Firmata-object). And actually I don't know whether it really is a good idea to clone an instance of an object that is meant to be a Singleton (since it represents a single pice of hardware). So from my point of view using a pointer as private member is the correct solution here.

Regarding this 'properly implement Stream' issue here: I don't know wether it's actually worth to implement all Stream-methods as long the only thing we actually need in Firmata-feature-classes seems to be 'write'. The code that is subclass-specific is also the code calling Firmata.begin(&Stream), so it has access to the instance of Stream anyway.

[soundanalogous]

ntruchsess code has been merged into the dev branch. I'm opening it up for review and further testing on the firmata dev list. If there are no objections I'll merge this into master on Feb 11th.

[soundanalogous]

Tested successfully with Serial1 on Mega and Leonardo and also with SoftwareSerial.