Skip to content

Support for all cyclic messages #15

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

Merged
merged 4 commits into from
Apr 24, 2025

Conversation

BrechtSerckx
Copy link
Contributor

This is a fix for #13.
Example program: https://gist.github.com/BrechtSerckx/6d23b859b3192aa284aa145c497aafe5

This will probably break the example program in the library unless #14 is merged in, as currently the callback for GetBusVI will intercept explicitly requested messages.

@BrechtSerckx BrechtSerckx marked this pull request as ready for review April 23, 2025 20:10
@CLAassistant
Copy link

CLAassistant commented Apr 24, 2025

CLA assistant check
All committers have signed the CLA.

@samuelsadok
Copy link
Member

samuelsadok commented Apr 24, 2025

As the number of callbacks increases, maybe we should consider other ways to scale this:

  • Leaving it as-is. The mental model here is that each callback function is independent and consists of two pointer-sized variables: the function pointer and a context pointer.
  • Same mental model as above, but collect function pointer and context pointer into a single Callback struct for easier handling and type safety. I'm a bit afraid of the template errors this could confront a user with.
  • Replacing all [...]_user_data fields into a single user_data field that is set once with ODriveCAN::setUserData().
  • Adding a base class of the form
    class ODriveDelegate {
        virtual void onMessage(const Heartbeat_msg_t&) {}
        virtual void onMessage(const Get_Encoder_Estimates_msg_t&) {}
        // ...
    }
    A user could implement this class (only the callbacks they need) and then only has to register a single instance of their delegate.
  • Storing the latest copy of each supported RX message type as part of ODriveCAN, along with a user-resettable received flag for each, and only have one single onMessage() callback that is invoked for every incoming message. This is similar to what the example code currently does, but the additional member fields on ODriveCAN might be superfluous in the general case.

The first option requires no user-code changes, the last option requires the most user-code changes.
Options 4 and 5 could be combined by providing a default ODriveDelegate implementation.

@BrechtSerckx would your real-world use case work well with option 4 or 5?
@Wetmelon what's your take?

@BrechtSerckx
Copy link
Contributor Author

@BrechtSerckx would your real-world use case work well with option 4 or 5? @Wetmelon what's your take?

As far as I can see, yes. My use-case is very similar to the example code.

Small disclaimer: while I find the alternative options interesting, I can't say when I'll have time to work on that, also considering my very limited C++ knowledge. Would it be possible to merge as option 1 and refactor as a follow-up at some point?

@samuelsadok
Copy link
Member

Ok yeah it makes sense to segment the refactoring into a separate PR. I'll add a TODO after merging with a link to this conversation.

@samuelsadok samuelsadok merged commit e4b1976 into odriverobotics:master Apr 24, 2025
3 checks passed
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