Check requested message before trying callbacks

[BrechtSerckx]

Hi,

I encountered this while working on #13.
Here is an example program to reproduce it: https://gist.github.com/BrechtSerckx/e682a30fb946292be58ab2be64ed49e3.
When no feedback handler is registered, calls to getFeedback fail. When a feedback handler is registered, calls still fail, but the callback is called.
getFeedback should not conflict with registered callbacks.

This patch first checks if a CAN message has been requested. Only if it wasn't, the callback handlers are checked.

[CLAassistant]

All committers have signed the CLA.

[samuelsadok]

Intent and implementation looks good to me.

Technically it could be a breaking change for someone but the previous behavior seems a bit quirky to rely on and we can bump the library version to 0.11.

Do you mind doing a rebase & force-push to trigger the new CI test? (see comment on #12)

[BrechtSerckx]

Do you mind doing a rebase & force-push to trigger the new CI test? (see comment on #12)

I've rebased on master and force-pushed, but nothing seems to have been triggered. Could it be that the workflows need approval?

[samuelsadok]

Thanks! Yes the workflows need approval to run.

[proan]

This definitely broke my code. The new behavior is more inline with the expectation of how the request() function will behave.

It is absolutely a breaking change, and it is missing from the CHANGELOG.

[samuelsadok]

Can you give an minimal example of the code that broke?
What hint in the changelog would have helped you?

[proan]

ODriveUserData odrive_user_data;
Heartbeat_msg_t heartbeat_msg;
while ( !odrive_user_data.received_heartbeat )
{
if ( !this->request( heartbeat_msg, 1000 ) )
{
Serial.println( "Heartbeat request failed!" );
}
else
{
Serial.print( "*" );
}
}

In 0.10.6 the above code will print an error message (missing callback) unless a heartbeat callback is created and registered:

void onHeartbeat( Heartbeat_msg_t& msg, void* user_data )
{
ODriveUserData* odrive_user_data = static_cast<ODriveUserData*>( user_data );
odrive_user_data->last_heartbeat = msg;
odrive_user_data->received_heartbeat = true;
}
ODriveCAN odrive0;
odrive0.onStatus( onHeartbeat, &motorController.odrive_user_data );

When the callback is registered, the code above works as expected to confirm the ODrive is connected. In 0.10.9, this code fails to exit the while() loop because the heartbeat callback is not called from onRecieve(). The onReceive() behavior has changed so that it exits when a CAN message from the ODrive is a response to a request() before it hands the message off to the registered callbacks.

To achieve the code above in 0.10.9 the loop looks like this:

ODriveUserData odrive_user_data;
Heartbeat_msg_t heartbeat_msg;
while ( !odrive_user_data.received_heartbeat )
{
if ( !this->request( heartbeat_msg, 1000 ) )
{
Serial.println( "Heartbeat request failed!" );
}
else
{
odrive_user_data.received_heartbeat = true;
}
}

The behavior in 0.10.9 is more uniform in that callbacks are only triggered when the message from the ODrive comes in without being requested by the Arduino. In my opinion, this is an improvement.

As for what I would like to see in the changelog, I think a line about the new request() behavior would be helpful. Something like "ODrive responses to requested messages (using request()) no longer trigger callbacks."

[BrechtSerckx]

Hi,

I encountered this while working on #13.
Here is an example program to reproduce it: https://gist.github.com/BrechtSerckx/e682a30fb946292be58ab2be64ed49e3.
When no feedback handler is registered, calls to getFeedback fail. When a feedback handler is registered, calls still fail, but the callback is called.
getFeedback should not conflict with registered callbacks.

This patch first checks if a CAN message has been requested. Only if it wasn't, the callback handlers are checked.

[CLAassistant]

All committers have signed the CLA.

[samuelsadok]

Intent and implementation looks good to me.

Technically it could be a breaking change for someone but the previous behavior seems a bit quirky to rely on and we can bump the library version to 0.11.

Do you mind doing a rebase & force-push to trigger the new CI test? (see comment on #12)

[BrechtSerckx]

Do you mind doing a rebase & force-push to trigger the new CI test? (see comment on #12)

I've rebased on master and force-pushed, but nothing seems to have been triggered. Could it be that the workflows need approval?

[samuelsadok]

Thanks! Yes the workflows need approval to run.

[proan]

This definitely broke my code. The new behavior is more inline with the expectation of how the request() function will behave.

It is absolutely a breaking change, and it is missing from the CHANGELOG.

[samuelsadok]

Can you give an minimal example of the code that broke?
What hint in the changelog would have helped you?

[proan]

ODriveUserData odrive_user_data;
Heartbeat_msg_t heartbeat_msg;
while ( !odrive_user_data.received_heartbeat )
{
if ( !this->request( heartbeat_msg, 1000 ) )
{
Serial.println( "Heartbeat request failed!" );
}
else
{
Serial.print( "*" );
}
}

In 0.10.6 the above code will print an error message (missing callback) unless a heartbeat callback is created and registered:

void onHeartbeat( Heartbeat_msg_t& msg, void* user_data )
{
ODriveUserData* odrive_user_data = static_cast<ODriveUserData*>( user_data );
odrive_user_data->last_heartbeat = msg;
odrive_user_data->received_heartbeat = true;
}
ODriveCAN odrive0;
odrive0.onStatus( onHeartbeat, &motorController.odrive_user_data );

When the callback is registered, the code above works as expected to confirm the ODrive is connected. In 0.10.9, this code fails to exit the while() loop because the heartbeat callback is not called from onRecieve(). The onReceive() behavior has changed so that it exits when a CAN message from the ODrive is a response to a request() before it hands the message off to the registered callbacks.

To achieve the code above in 0.10.9 the loop looks like this:

ODriveUserData odrive_user_data;
Heartbeat_msg_t heartbeat_msg;
while ( !odrive_user_data.received_heartbeat )
{
if ( !this->request( heartbeat_msg, 1000 ) )
{
Serial.println( "Heartbeat request failed!" );
}
else
{
odrive_user_data.received_heartbeat = true;
}
}

The behavior in 0.10.9 is more uniform in that callbacks are only triggered when the message from the ODrive comes in without being requested by the Arduino. In my opinion, this is an improvement.

As for what I would like to see in the changelog, I think a line about the new request() behavior would be helpful. Something like "ODrive responses to requested messages (using request()) no longer trigger callbacks."

[BrechtSerckx]

Hi,

I encountered this while working on #13.
Here is an example program to reproduce it: https://gist.github.com/BrechtSerckx/e682a30fb946292be58ab2be64ed49e3.
When no feedback handler is registered, calls to getFeedback fail. When a feedback handler is registered, calls still fail, but the callback is called.
getFeedback should not conflict with registered callbacks.

This patch first checks if a CAN message has been requested. Only if it wasn't, the callback handlers are checked.

[CLAassistant]

All committers have signed the CLA.

[samuelsadok]

Intent and implementation looks good to me.

Technically it could be a breaking change for someone but the previous behavior seems a bit quirky to rely on and we can bump the library version to 0.11.

Do you mind doing a rebase & force-push to trigger the new CI test? (see comment on #12)

[BrechtSerckx]

Do you mind doing a rebase & force-push to trigger the new CI test? (see comment on #12)

I've rebased on master and force-pushed, but nothing seems to have been triggered. Could it be that the workflows need approval?

[samuelsadok]

Thanks! Yes the workflows need approval to run.

[proan]

This definitely broke my code. The new behavior is more inline with the expectation of how the request() function will behave.

It is absolutely a breaking change, and it is missing from the CHANGELOG.

[samuelsadok]

Can you give an minimal example of the code that broke?
What hint in the changelog would have helped you?

[proan]

ODriveUserData odrive_user_data;
Heartbeat_msg_t heartbeat_msg;
while ( !odrive_user_data.received_heartbeat )
{
if ( !this->request( heartbeat_msg, 1000 ) )
{
Serial.println( "Heartbeat request failed!" );
}
else
{
Serial.print( "*" );
}
}

In 0.10.6 the above code will print an error message (missing callback) unless a heartbeat callback is created and registered:

void onHeartbeat( Heartbeat_msg_t& msg, void* user_data )
{
ODriveUserData* odrive_user_data = static_cast<ODriveUserData*>( user_data );
odrive_user_data->last_heartbeat = msg;
odrive_user_data->received_heartbeat = true;
}
ODriveCAN odrive0;
odrive0.onStatus( onHeartbeat, &motorController.odrive_user_data );

When the callback is registered, the code above works as expected to confirm the ODrive is connected. In 0.10.9, this code fails to exit the while() loop because the heartbeat callback is not called from onRecieve(). The onReceive() behavior has changed so that it exits when a CAN message from the ODrive is a response to a request() before it hands the message off to the registered callbacks.

To achieve the code above in 0.10.9 the loop looks like this:

ODriveUserData odrive_user_data;
Heartbeat_msg_t heartbeat_msg;
while ( !odrive_user_data.received_heartbeat )
{
if ( !this->request( heartbeat_msg, 1000 ) )
{
Serial.println( "Heartbeat request failed!" );
}
else
{
odrive_user_data.received_heartbeat = true;
}
}

The behavior in 0.10.9 is more uniform in that callbacks are only triggered when the message from the ODrive comes in without being requested by the Arduino. In my opinion, this is an improvement.

As for what I would like to see in the changelog, I think a line about the new request() behavior would be helpful. Something like "ODrive responses to requested messages (using request()) no longer trigger callbacks."

[BrechtSerckx]

Hi,

I encountered this while working on #13.
Here is an example program to reproduce it: https://gist.github.com/BrechtSerckx/e682a30fb946292be58ab2be64ed49e3.
When no feedback handler is registered, calls to getFeedback fail. When a feedback handler is registered, calls still fail, but the callback is called.
getFeedback should not conflict with registered callbacks.

This patch first checks if a CAN message has been requested. Only if it wasn't, the callback handlers are checked.

[CLAassistant]

All committers have signed the CLA.

[samuelsadok]

Intent and implementation looks good to me.

Technically it could be a breaking change for someone but the previous behavior seems a bit quirky to rely on and we can bump the library version to 0.11.

Do you mind doing a rebase & force-push to trigger the new CI test? (see comment on #12)

[BrechtSerckx]

Do you mind doing a rebase & force-push to trigger the new CI test? (see comment on #12)

I've rebased on master and force-pushed, but nothing seems to have been triggered. Could it be that the workflows need approval?

[samuelsadok]

Thanks! Yes the workflows need approval to run.

[proan]

This definitely broke my code. The new behavior is more inline with the expectation of how the request() function will behave.

It is absolutely a breaking change, and it is missing from the CHANGELOG.

[samuelsadok]

Can you give an minimal example of the code that broke?
What hint in the changelog would have helped you?

[proan]

ODriveUserData odrive_user_data;
Heartbeat_msg_t heartbeat_msg;
while ( !odrive_user_data.received_heartbeat )
{
if ( !this->request( heartbeat_msg, 1000 ) )
{
Serial.println( "Heartbeat request failed!" );
}
else
{
Serial.print( "*" );
}
}

In 0.10.6 the above code will print an error message (missing callback) unless a heartbeat callback is created and registered:

void onHeartbeat( Heartbeat_msg_t& msg, void* user_data )
{
ODriveUserData* odrive_user_data = static_cast<ODriveUserData*>( user_data );
odrive_user_data->last_heartbeat = msg;
odrive_user_data->received_heartbeat = true;
}
ODriveCAN odrive0;
odrive0.onStatus( onHeartbeat, &motorController.odrive_user_data );

When the callback is registered, the code above works as expected to confirm the ODrive is connected. In 0.10.9, this code fails to exit the while() loop because the heartbeat callback is not called from onRecieve(). The onReceive() behavior has changed so that it exits when a CAN message from the ODrive is a response to a request() before it hands the message off to the registered callbacks.

To achieve the code above in 0.10.9 the loop looks like this:

ODriveUserData odrive_user_data;
Heartbeat_msg_t heartbeat_msg;
while ( !odrive_user_data.received_heartbeat )
{
if ( !this->request( heartbeat_msg, 1000 ) )
{
Serial.println( "Heartbeat request failed!" );
}
else
{
odrive_user_data.received_heartbeat = true;
}
}

The behavior in 0.10.9 is more uniform in that callbacks are only triggered when the message from the ODrive comes in without being requested by the Arduino. In my opinion, this is an improvement.

As for what I would like to see in the changelog, I think a line about the new request() behavior would be helpful. Something like "ODrive responses to requested messages (using request()) no longer trigger callbacks."

[BrechtSerckx]

Hi,

I encountered this while working on #13.
Here is an example program to reproduce it: https://gist.github.com/BrechtSerckx/e682a30fb946292be58ab2be64ed49e3.
When no feedback handler is registered, calls to getFeedback fail. When a feedback handler is registered, calls still fail, but the callback is called.
getFeedback should not conflict with registered callbacks.

This patch first checks if a CAN message has been requested. Only if it wasn't, the callback handlers are checked.

[CLAassistant]

All committers have signed the CLA.

[samuelsadok]

Intent and implementation looks good to me.

Technically it could be a breaking change for someone but the previous behavior seems a bit quirky to rely on and we can bump the library version to 0.11.

Do you mind doing a rebase & force-push to trigger the new CI test? (see comment on #12)

[BrechtSerckx]

Do you mind doing a rebase & force-push to trigger the new CI test? (see comment on #12)

I've rebased on master and force-pushed, but nothing seems to have been triggered. Could it be that the workflows need approval?

[samuelsadok]

Thanks! Yes the workflows need approval to run.

[proan]

This definitely broke my code. The new behavior is more inline with the expectation of how the request() function will behave.

It is absolutely a breaking change, and it is missing from the CHANGELOG.

[samuelsadok]

Can you give an minimal example of the code that broke?
What hint in the changelog would have helped you?

[proan]

ODriveUserData odrive_user_data;
Heartbeat_msg_t heartbeat_msg;
while ( !odrive_user_data.received_heartbeat )
{
if ( !this->request( heartbeat_msg, 1000 ) )
{
Serial.println( "Heartbeat request failed!" );
}
else
{
Serial.print( "*" );
}
}

In 0.10.6 the above code will print an error message (missing callback) unless a heartbeat callback is created and registered:

void onHeartbeat( Heartbeat_msg_t& msg, void* user_data )
{
ODriveUserData* odrive_user_data = static_cast<ODriveUserData*>( user_data );
odrive_user_data->last_heartbeat = msg;
odrive_user_data->received_heartbeat = true;
}
ODriveCAN odrive0;
odrive0.onStatus( onHeartbeat, &motorController.odrive_user_data );

When the callback is registered, the code above works as expected to confirm the ODrive is connected. In 0.10.9, this code fails to exit the while() loop because the heartbeat callback is not called from onRecieve(). The onReceive() behavior has changed so that it exits when a CAN message from the ODrive is a response to a request() before it hands the message off to the registered callbacks.

To achieve the code above in 0.10.9 the loop looks like this:

ODriveUserData odrive_user_data;
Heartbeat_msg_t heartbeat_msg;
while ( !odrive_user_data.received_heartbeat )
{
if ( !this->request( heartbeat_msg, 1000 ) )
{
Serial.println( "Heartbeat request failed!" );
}
else
{
odrive_user_data.received_heartbeat = true;
}
}

The behavior in 0.10.9 is more uniform in that callbacks are only triggered when the message from the ODrive comes in without being requested by the Arduino. In my opinion, this is an improvement.

As for what I would like to see in the changelog, I think a line about the new request() behavior would be helpful. Something like "ODrive responses to requested messages (using request()) no longer trigger callbacks."

[BrechtSerckx]

Hi,

I encountered this while working on #13.
Here is an example program to reproduce it: https://gist.github.com/BrechtSerckx/e682a30fb946292be58ab2be64ed49e3.
When no feedback handler is registered, calls to getFeedback fail. When a feedback handler is registered, calls still fail, but the callback is called.
getFeedback should not conflict with registered callbacks.

This patch first checks if a CAN message has been requested. Only if it wasn't, the callback handlers are checked.

[CLAassistant]

All committers have signed the CLA.

[samuelsadok]

Intent and implementation looks good to me.

Technically it could be a breaking change for someone but the previous behavior seems a bit quirky to rely on and we can bump the library version to 0.11.

Do you mind doing a rebase & force-push to trigger the new CI test? (see comment on #12)

[BrechtSerckx]

Do you mind doing a rebase & force-push to trigger the new CI test? (see comment on #12)

I've rebased on master and force-pushed, but nothing seems to have been triggered. Could it be that the workflows need approval?

[samuelsadok]

Thanks! Yes the workflows need approval to run.

[proan]

This definitely broke my code. The new behavior is more inline with the expectation of how the request() function will behave.

It is absolutely a breaking change, and it is missing from the CHANGELOG.

[samuelsadok]

Can you give an minimal example of the code that broke?
What hint in the changelog would have helped you?

[proan]

ODriveUserData odrive_user_data;
Heartbeat_msg_t heartbeat_msg;
while ( !odrive_user_data.received_heartbeat )
{
if ( !this->request( heartbeat_msg, 1000 ) )
{
Serial.println( "Heartbeat request failed!" );
}
else
{
Serial.print( "*" );
}
}

In 0.10.6 the above code will print an error message (missing callback) unless a heartbeat callback is created and registered:

void onHeartbeat( Heartbeat_msg_t& msg, void* user_data )
{
ODriveUserData* odrive_user_data = static_cast<ODriveUserData*>( user_data );
odrive_user_data->last_heartbeat = msg;
odrive_user_data->received_heartbeat = true;
}
ODriveCAN odrive0;
odrive0.onStatus( onHeartbeat, &motorController.odrive_user_data );

When the callback is registered, the code above works as expected to confirm the ODrive is connected. In 0.10.9, this code fails to exit the while() loop because the heartbeat callback is not called from onRecieve(). The onReceive() behavior has changed so that it exits when a CAN message from the ODrive is a response to a request() before it hands the message off to the registered callbacks.

To achieve the code above in 0.10.9 the loop looks like this:

ODriveUserData odrive_user_data;
Heartbeat_msg_t heartbeat_msg;
while ( !odrive_user_data.received_heartbeat )
{
if ( !this->request( heartbeat_msg, 1000 ) )
{
Serial.println( "Heartbeat request failed!" );
}
else
{
odrive_user_data.received_heartbeat = true;
}
}

The behavior in 0.10.9 is more uniform in that callbacks are only triggered when the message from the ODrive comes in without being requested by the Arduino. In my opinion, this is an improvement.

As for what I would like to see in the changelog, I think a line about the new request() behavior would be helpful. Something like "ODrive responses to requested messages (using request()) no longer trigger callbacks."

[BrechtSerckx]

Hi,

I encountered this while working on #13.
Here is an example program to reproduce it: https://gist.github.com/BrechtSerckx/e682a30fb946292be58ab2be64ed49e3.
When no feedback handler is registered, calls to getFeedback fail. When a feedback handler is registered, calls still fail, but the callback is called.
getFeedback should not conflict with registered callbacks.

This patch first checks if a CAN message has been requested. Only if it wasn't, the callback handlers are checked.

[CLAassistant]

All committers have signed the CLA.

[samuelsadok]

Intent and implementation looks good to me.

Technically it could be a breaking change for someone but the previous behavior seems a bit quirky to rely on and we can bump the library version to 0.11.

Do you mind doing a rebase & force-push to trigger the new CI test? (see comment on #12)

[BrechtSerckx]

Do you mind doing a rebase & force-push to trigger the new CI test? (see comment on #12)

I've rebased on master and force-pushed, but nothing seems to have been triggered. Could it be that the workflows need approval?

[samuelsadok]

Thanks! Yes the workflows need approval to run.

[proan]

This definitely broke my code. The new behavior is more inline with the expectation of how the request() function will behave.

It is absolutely a breaking change, and it is missing from the CHANGELOG.

[samuelsadok]

Can you give an minimal example of the code that broke?
What hint in the changelog would have helped you?

[proan]

ODriveUserData odrive_user_data;
Heartbeat_msg_t heartbeat_msg;
while ( !odrive_user_data.received_heartbeat )
{
if ( !this->request( heartbeat_msg, 1000 ) )
{
Serial.println( "Heartbeat request failed!" );
}
else
{
Serial.print( "*" );
}
}

In 0.10.6 the above code will print an error message (missing callback) unless a heartbeat callback is created and registered:

void onHeartbeat( Heartbeat_msg_t& msg, void* user_data )
{
ODriveUserData* odrive_user_data = static_cast<ODriveUserData*>( user_data );
odrive_user_data->last_heartbeat = msg;
odrive_user_data->received_heartbeat = true;
}
ODriveCAN odrive0;
odrive0.onStatus( onHeartbeat, &motorController.odrive_user_data );

When the callback is registered, the code above works as expected to confirm the ODrive is connected. In 0.10.9, this code fails to exit the while() loop because the heartbeat callback is not called from onRecieve(). The onReceive() behavior has changed so that it exits when a CAN message from the ODrive is a response to a request() before it hands the message off to the registered callbacks.

To achieve the code above in 0.10.9 the loop looks like this:

ODriveUserData odrive_user_data;
Heartbeat_msg_t heartbeat_msg;
while ( !odrive_user_data.received_heartbeat )
{
if ( !this->request( heartbeat_msg, 1000 ) )
{
Serial.println( "Heartbeat request failed!" );
}
else
{
odrive_user_data.received_heartbeat = true;
}
}

The behavior in 0.10.9 is more uniform in that callbacks are only triggered when the message from the ODrive comes in without being requested by the Arduino. In my opinion, this is an improvement.

As for what I would like to see in the changelog, I think a line about the new request() behavior would be helpful. Something like "ODrive responses to requested messages (using request()) no longer trigger callbacks."

[BrechtSerckx]

Hi,

I encountered this while working on #13.
Here is an example program to reproduce it: https://gist.github.com/BrechtSerckx/e682a30fb946292be58ab2be64ed49e3.
When no feedback handler is registered, calls to getFeedback fail. When a feedback handler is registered, calls still fail, but the callback is called.
getFeedback should not conflict with registered callbacks.

This patch first checks if a CAN message has been requested. Only if it wasn't, the callback handlers are checked.

[CLAassistant]

All committers have signed the CLA.

[samuelsadok]

Intent and implementation looks good to me.

Technically it could be a breaking change for someone but the previous behavior seems a bit quirky to rely on and we can bump the library version to 0.11.

Do you mind doing a rebase & force-push to trigger the new CI test? (see comment on #12)

[BrechtSerckx]

Do you mind doing a rebase & force-push to trigger the new CI test? (see comment on #12)

I've rebased on master and force-pushed, but nothing seems to have been triggered. Could it be that the workflows need approval?

[samuelsadok]

Thanks! Yes the workflows need approval to run.

[proan]

This definitely broke my code. The new behavior is more inline with the expectation of how the request() function will behave.

It is absolutely a breaking change, and it is missing from the CHANGELOG.

[samuelsadok]

Can you give an minimal example of the code that broke?
What hint in the changelog would have helped you?

[proan]

ODriveUserData odrive_user_data;
Heartbeat_msg_t heartbeat_msg;
while ( !odrive_user_data.received_heartbeat )
{
if ( !this->request( heartbeat_msg, 1000 ) )
{
Serial.println( "Heartbeat request failed!" );
}
else
{
Serial.print( "*" );
}
}

In 0.10.6 the above code will print an error message (missing callback) unless a heartbeat callback is created and registered:

void onHeartbeat( Heartbeat_msg_t& msg, void* user_data )
{
ODriveUserData* odrive_user_data = static_cast<ODriveUserData*>( user_data );
odrive_user_data->last_heartbeat = msg;
odrive_user_data->received_heartbeat = true;
}
ODriveCAN odrive0;
odrive0.onStatus( onHeartbeat, &motorController.odrive_user_data );

When the callback is registered, the code above works as expected to confirm the ODrive is connected. In 0.10.9, this code fails to exit the while() loop because the heartbeat callback is not called from onRecieve(). The onReceive() behavior has changed so that it exits when a CAN message from the ODrive is a response to a request() before it hands the message off to the registered callbacks.

To achieve the code above in 0.10.9 the loop looks like this:

ODriveUserData odrive_user_data;
Heartbeat_msg_t heartbeat_msg;
while ( !odrive_user_data.received_heartbeat )
{
if ( !this->request( heartbeat_msg, 1000 ) )
{
Serial.println( "Heartbeat request failed!" );
}
else
{
odrive_user_data.received_heartbeat = true;
}
}

The behavior in 0.10.9 is more uniform in that callbacks are only triggered when the message from the ODrive comes in without being requested by the Arduino. In my opinion, this is an improvement.

As for what I would like to see in the changelog, I think a line about the new request() behavior would be helpful. Something like "ODrive responses to requested messages (using request()) no longer trigger callbacks."