Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[v3.0.0] version callbacks #358

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[v3.0.0] version callbacks #358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Firmata.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ FirmataClass::FirmataClass()
parser.attach(STRING_DATA, (FirmataParser::stringCallbackFunction)staticStringCallback, (void *)NULL);
parser.attach(START_SYSEX, (FirmataParser::sysexCallbackFunction)staticSysexCallback, (void *)NULL);
parser.attach(REPORT_FIRMWARE, (FirmataParser::versionCallbackFunction)staticReportFirmwareCallback, this);
parser.attach(REPORT_VERSION, (FirmataParser::systemCallbackFunction)staticReportVersionCallback, this);
parser.attach(REPORT_VERSION, (FirmataParser::versionCallbackFunction)staticReportVersionCallback, this);
parser.attach(SYSTEM_RESET, (FirmataParser::systemCallbackFunction)staticSystemResetCallback, (void *)NULL);
}

Expand Down
2 changes: 1 addition & 1 deletion Firmata.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ class FirmataClass
inline static void staticStringCallback (void *, const char * c_str) { if ( currentStringCallback ) { currentStringCallback((char *)c_str); } }
inline static void staticSysexCallback (void *, uint8_t command, size_t argc, uint8_t *argv) { if ( currentSysexCallback ) { currentSysexCallback(command, (uint8_t)argc, argv); } }
inline static void staticReportFirmwareCallback (void * context, size_t, size_t, const char *) { if ( context ) { ((FirmataClass *)context)->printFirmwareVersion(); } }
inline static void staticReportVersionCallback (void * context) { if ( context ) { ((FirmataClass *)context)->printVersion(); } }
inline static void staticReportVersionCallback (void * context, size_t, size_t, const char *) { if ( context ) { ((FirmataClass *)context)->printVersion(); } }
inline static void staticSystemResetCallback (void *) { if ( currentSystemResetCallback ) { currentSystemResetCallback(); } }
};

Expand Down
2 changes: 2 additions & 0 deletions FirmataMarshaller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,8 @@ const
{
if ( (Stream *)NULL == FirmataStream ) { return; }
FirmataStream->write(REPORT_VERSION);
FirmataStream->write(PROTOCOL_MAJOR_VERSION);
FirmataStream->write(PROTOCOL_MINOR_VERSION);
}

/**
Expand Down
24 changes: 15 additions & 9 deletions FirmataParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ FirmataParser::FirmataParser(uint8_t * const dataBuffer, size_t dataBufferSize)
currentStringCallback((stringCallbackFunction)NULL),
currentSysexCallback((sysexCallbackFunction)NULL),
currentReportFirmwareCallback((versionCallbackFunction)NULL),
currentReportVersionCallback((systemCallbackFunction)NULL),
currentReportVersionCallback((versionCallbackFunction)NULL),
currentSystemResetCallback((systemCallbackFunction)NULL)
{
allowBufferUpdate = ((uint8_t *)NULL == dataBuffer);
Expand Down Expand Up @@ -130,6 +130,9 @@ void FirmataParser::parse(uint8_t inputData)
if (currentReportDigitalCallback)
(*currentReportDigitalCallback)(currentReportDigitalCallbackContext, multiByteChannel, dataBuffer[0]);
break;
case REPORT_VERSION:
if (currentReportVersionCallback)
(*currentReportVersionCallback)(currentReportVersionCallbackContext, dataBuffer[0], dataBuffer[1], (const char *)NULL);
Copy link
Member

@soundanalogous soundanalogous Mar 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Firmata client libraries do not provide data parameters for REPORT_VERSION so I'm not sure where the version data would come from. You could pass it in statically though. This is probably why the ESP8266 isn't working.

Copy link
Contributor Author

@zfields zfields Mar 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am aware, but I need to standardize the message so the parser can parse it - even if portions are disregarded. The name of the byte is REPORT version, not QUERY. You happen to be using it for both.

Copy link
Member

@soundanalogous soundanalogous Mar 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm starting to understand. Let me know if this is correct.

The version parameters are needed in the callback since the callback is actually for the reply rather than the query. When the callback is called, those parameters are available.

The confusion on the protocol side is that REPORT_VERSION and REPORT_FIRMWARE were shared for both query and reply (which was common in the early days of Firmata).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see since we can anticipate that dataBuffer[0] and dataBuffer[1] will contain the appropriate data when the callback is called. I was overlooking the fact that dataBuffer was a member variable, that's what was largely throwing me off. Maybe we should have a convention member variables. I've been prefixing with m for the SPI feature I've been working on: https://github.com/firmata/arduino/blob/spi-alpha/utility/SPIFirmata.h#L94-L96.

Copy link
Contributor Author

@zfields zfields Mar 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Originally, the parser was built into FirmataClass which serves the host/server.
  • There was no notion of a callback for version, ever; it was tightly coupled.
  • REPORT_VERSION was used to query the version from the host/server, and it would subsequently report the version using REPORT_VERSION.
  • The fact that REPORT_VERSION is a single byte operation in the protocol documentation is wrong, because the version is actually reported using three bytes.
  • Therefore, REPORT_VERSION should always send three bytes and the server/host should be smart enough realize it is being queried and discard them.
  • An alternate approach would be to create a single byte QUERY_VERSION used to explicitly query the version from the server/host
  • REPORT_FIRMWARE should follow a similar approach

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add QUERY_FIRMWARE_VERSION and QUERY_PROTOCOL_VERSION for v3.0 since I can't really think of a good reason for a client library to send it's version to the board.

Copy link
Contributor Author

@zfields zfields Mar 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefix all my _member variables on my projects, so I'm down. I was just following style.

}
executeMultiByteCommand = 0;
}
Expand Down Expand Up @@ -163,8 +166,8 @@ void FirmataParser::parse(uint8_t inputData)
systemReset();
break;
case REPORT_VERSION:
if (currentReportVersionCallback)
(*currentReportVersionCallback)(currentReportVersionCallbackContext);
waitForData = 2; // two data bytes needed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where I wish we had unit test coverage for the parser. I'm still not 100% sure this will not break with most existing Firmata client libraries.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd want to know that if waitForData is 2 or 0 this will still work as expected.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah tracing through the code, this will cause the next two bytes after a REPORT_VERSION command is sent (as a query) from a client library to be skipped. So if REPORT_VERSION is followed immediately by a Sysex message (which I believe is the case in firmata.js) then that entire Sysex message will be lost (since the START_SYSEX and COMMAND bytes will be skipped).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually START_SYSEX will get through because ( (waitForData > 0) && (inputData < 128) )
But I don't think the COMMAND byte will because waitForData would still equal 2. One work around is in the START_SYSEX case, reset waitForData to 0.

Copy link
Contributor Author

@zfields zfields Mar 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does change the behavior... With this change, you will need to send all three bytes for the version in order to get the callback from FirmataClass to fire. However, it is worth noting that the unsolicited sketch startup REPORT_VERSION message fires, regardless.

Conceivably REPORT_FIRMWARE should work the same either way. On the host side, would just pull some junk out of the buffer for the version info, but it would turn around and discard it immediately.

Perhaps it would be good to split these up. It would allow us to pull in REPORT_FIRMWARE and leave REPORT_VERSION outstanding for v3.0.0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's going to break a lot of client libraries. We'll have to wait for Firmata v3.0 then.

Copy link
Member

@soundanalogous soundanalogous Mar 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because many Firmata client libraries initiate communication with the board by querying the protocol version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is some general confusion here because REPORT_VERSION is misleading. Basically a client should never report its version to the host/firmware. A client can only query the firmware and protocol versions from the host/firmware.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you're trying to use the same parser both on the host and client side so now I understand the larger issue here.

executeMultiByteCommand = command;
break;
}
}
Expand Down Expand Up @@ -244,12 +247,13 @@ void FirmataParser::attach(uint8_t command, callbackFunction newFunction, void *
}

/**
* Attach a version callback function (supported option: REPORT_FIRMWARE).
* Attach a version callback function (supported options are: REPORT_FIRMWARE, REPORT_VERSION).
* @param command The ID of the command to attach a callback function to.
* @param newFunction A reference to the callback function to attach.
* @param context An optional context to be provided to the callback function (NULL by default).
* @note The context parameter is provided so you can pass a parameter, by reference, to
* your callback function.
* @note The description value in the REPORT_VERSION callback will always be NULL
*/
void FirmataParser::attach(uint8_t command, versionCallbackFunction newFunction, void * context)
{
Expand All @@ -258,11 +262,15 @@ void FirmataParser::attach(uint8_t command, versionCallbackFunction newFunction,
currentReportFirmwareCallback = newFunction;
currentReportFirmwareCallbackContext = context;
break;
case REPORT_VERSION:
currentReportVersionCallback = newFunction;
currentReportVersionCallbackContext = context;
break;
}
}

/**
* Attach a system callback function (supported options are: SYSTEM_RESET, REPORT_VERSION).
* Attach a system callback function (supported option: SYSTEM_RESET).
* @param command The ID of the command to attach a callback function to.
* @param newFunction A reference to the callback function to attach.
* @param context An optional context to be provided to the callback function (NULL by default).
Expand All @@ -272,10 +280,6 @@ void FirmataParser::attach(uint8_t command, versionCallbackFunction newFunction,
void FirmataParser::attach(uint8_t command, systemCallbackFunction newFunction, void * context)
{
switch (command) {
case REPORT_VERSION:
currentReportVersionCallback = newFunction;
currentReportVersionCallbackContext = context;
break;
case SYSTEM_RESET:
currentSystemResetCallback = newFunction;
currentSystemResetCallbackContext = context;
Expand Down Expand Up @@ -341,6 +345,8 @@ void FirmataParser::detach(uint8_t command)
attach(command, (versionCallbackFunction)NULL, NULL);
break;
case REPORT_VERSION:
attach(command, (versionCallbackFunction)NULL, NULL);
break;
case SYSTEM_RESET:
attach(command, (systemCallbackFunction)NULL, NULL);
break;
Expand Down
2 changes: 1 addition & 1 deletion FirmataParser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class FirmataParser
stringCallbackFunction currentStringCallback;
sysexCallbackFunction currentSysexCallback;
versionCallbackFunction currentReportFirmwareCallback;
systemCallbackFunction currentReportVersionCallback;
versionCallbackFunction currentReportVersionCallback;
systemCallbackFunction currentSystemResetCallback;

/* private methods ------------------------------ */
Expand Down