FirmataMarshaller and FirmataParser are incomplete

[zfields]

Previously, we discussed that FirmataMarshaller and FirmataParser should only contain the core functionality and nothing more. Then in the future (think configurable Firmata) we would use the marshaller and parser as a base to extend.

Would you enumerate the core functionality missing from the marshaller and parser here, so I may use it as a checklist to implement the remaining methods?

[zfields]

As an example... sendPinStateQuery is missing from FirmataMarshaller

[zfields]

@soundanalogous I would love your input on this list.

[soundanalogous]

I'll work on the list sometime this weekend. However one thing this makes me think about is now that we have FirmataMarshaller if it makes sense to handle any Sysex commands in FirmataParser anymore. For example, could we move STRING_DATA and REPORT_FIRMWARE (https://github.com/firmata/arduino/blob/master/FirmataParser.cpp#L394-L420) out of FirmataParser and handle them in FirmataMarshaller? That would keep FirmataParser more in line with midi and FirmataMarshaller handles the second layer protocol which is everything composed within sysex messages.

[soundanalogous]

The following core functionality is not yet implemented in FirmataMarshaller:

• [x] sendPinStateQuery
• [x] sendExtendedAnalog (could alternatively combine within sendAnalog and then only add a private sendExtendedAnalog method)
• [x] setSamplingInterval

[zfields]

I think it would be good to move STRING_DATA and REPORT_FIRMWARE out of FirmataParser and up into FirmataClass. This would allow for the elimination of the callbacks that belong to these.

However, I don't think they have a home in FirmataMarshaller.

[soundanalogous]

I guess one advantage of keeping STRING_DATA and REPORT_FIRMWARE out of FirmataClass is they don't depend on any particular hardware, so maybe it's best to keep them in FirmataParser for now then.

[zfields]

I think it depends on if you are planning to make callbacks for all known messages (especially core functionality), or if you want to allow the user to implement their own callback system through the sysex callback.

Right now in my project, I am using the callbacks that are available, but when they are not defined (i.e. capQuery, analogMapping, etc.) I'm not really having any problem capturing my sysex messages and parsing them myself using the constants in FirmataConstants.h.

That being said, I would prefer to see us add to the callback support as opposed to removing it. The only drawback I can think of, is that we would have to keep state for the callback and context, but we are talking about a handful of pointers (5-8 pair) beyond what we would have to support anyway. I think the benefit of usability would outweigh the cost of allocated memory.

[soundanalogous]

It probably makes sense only for core functionality. I'd assume some functionality could even be removed from StandardFirmata completely (and moved to FirmataMarshaller - unless I'm totally misunderstanding something about the role of the marshaller), which will greatly simplify things since there are so many StandardFirmata variants... the more shared code the better IMO. For optional features, each should be a class that handles the sysex callback. See SerialFirmata for an example.

[zfields]

The marshaller should only transform your intent into protocol defined byte patterns, and the parser would receive incoming bytes and change those into actions via callbacks. You would need another layer that is more akin to your FirmataClass that has implementation specific details.

Once I have the prototype up and running for RemoteWiring, you can look into my FirmataDevice class which has both a marshaller and parser and keeps track of the state of the board.

[zfields]

@soundanalogous Just wanted to follow up on this. Should we expand FirmataParser to provide individual callbacks for the FirmataMarshaller calls (would add two or three callbacks)?

[soundanalogous]

Which calls specifically?

[zfields]

FirmataMarshaller::sendCapabilityQuery -> FirmataParser::capabilityQueryCallback FirmataMarshaller::sendAnalogMappingQuery -> FirmataParser::analogMappingQueryCallback FirmataMarshaller::sendPinStateQuery -> FirmataParser::pinStateQueryCallback

Also, it would be good to update the existing version callbacks, as called out in #354 #355