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.

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

Handles unhandled-addressed-messages by generating an OIR reply. #798

Merged
merged 18 commits into from
Sep 7, 2024

Conversation

balazsracz
Copy link
Collaborator

@balazsracz balazsracz commented Aug 23, 2024

This PR fixes a stadnards compliance issue. The standard requires that when an addressed message arrives for a node which is not equipped for handling said message (including any unknown but addressed MTI arriving), then an Optional IOnteraction Rejected (OIR) message has to be sent back to the originating node, identifying that this target node is not able to handle that message.

  • Adds a "fallback handler" to the Dispatcher object. The fallback handler is invoked when no handler matched on the incoming message.
  • Implements a handler that responds with an OIR for all local nodes. Registers this handler as the fallback handler for the MessageDIspatcher in IfCan and IfTcp. This OIR response will be suppressed for incoming OIR and TDE errors to avoid infinite loops of error messages. (In a normal setup OpenMRN does not listen to these messages. Without this case two OpenMRN nodes will have an infinite message loop chatting with each other sending OIR messages back and forth.)

Fixes some bugs exposed by this behavior.

  • The TCP parser had forgotten to clear the message it was filling, causing messages to unexpectedly be marked as addressed messages.
  • A number of tests had issues when OIR messages were generated, this PR fixes these issues as well. Sometimes the MTI was mistyped, sometimes a message was verified at the CAN level but not actually processed at the destination node.

/// matching the MTI / mask of any existing instantiated handler will end up
/// here.
///
/// THe standard requires that when an addressed message is not handled by a
Copy link
Collaborator

Choose a reason for hiding this comment

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

THe -> The

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

iface()->addressed_message_write_flow(), STATE(fill_oir));
}

Action fill_oir()
Copy link
Owner

Choose a reason for hiding this comment

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

Could we add Doxygen header blocks to this method, and the others that are undocumented in this file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

/// This is an addressed message handler that catches errors (OIR and TDE) but
/// ignores them. While this sounds not super useful, it ensures that these
/// messages never trigger the UnhandledAddressedMessageHandler.
class NullErrorHandler : public IncomingMessageStateFlow
Copy link
Owner

Choose a reason for hiding this comment

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

This does seem super not useful because the party that initiated the interaction will not be notified, it just times out. Should we have some way of notifying the initiator of the interaction? Perhaps at least file this as an enhancement request issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Handlers that are registered are independent of each other, even if they listen to the same MTI. So if a particular component wants to know about OIR and TDE, it can (and should) register handlers for them. There are some examples of this, such as the PIPClient: https://github.com/bakerstu/openmrn/blob/master/src/openlcb/PIPClient.hxx#L159

There can be more than one handler for a given MTI. If there are multiple handlers for OIR/TDE, then they will each get the message (in a separate Buffer).

As a conclusion, the existence of this component has nothing to do with whether any other component chooses to listen to and act on OIR/TDE. It only needs said other component to write the code for handling the error messages. Note that this is typically interesting for client/UI components, not for services. Most of our code is service implementations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I actually deleted this handler and replaced it with an if (mti != OIR && mti != TDE) in the other handler. This has the same result but takes less RAM and FLASH space.

@balazsracz balazsracz merged commit 5ad34b1 into master Sep 7, 2024
4 checks passed
@balazsracz balazsracz deleted the bracz-nohandler-oir branch September 7, 2024 12:00
balazsracz added a commit that referenced this pull request Jan 16, 2025
* master: (76 commits)
  Fixes some compile errors in nucleo and bracz.acc.
  ESP-IDF CMakeLists (#800)
  BLE Basic Infrastructure (#788)
  Handles unhandled-addressed-messages by generating an OIR reply. (#798)
  Adds factory reset handler to linux:io_board. (#797)
  Fixes a standards compliance issue with the alias conflict handler. (#793)
  Bug fixes in DataBuffer (#791)
  Fixes race conditions in HubDeviceSelect. (#795)
  Fixes missing translation of enums when reading the security mode from a simplelink profile. (#796)
  Fixes flaky IfCanStress.test. (#794)
  Pin esp32 platform to 2.0.x (#792)
  Fixes detecting EOF in the memory config protocol handler. (#789)
  Adds a new hub application using DirectHub (#761)
  High-performance hub component for dealing with many sockets and high throughput (#760)
  Fix build of esp8266 train implementation.
  Removes unnecessary includes that might not exist on an embedded compiler.
  Fix compilation of TempFile under esp8266.
  Add libatomic to esp8266 nonos target.
  Fix compile errors in time_client app.
  Fixes in file memory space: (#786)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants