Skip to content

Conversation

@EhlOps
Copy link

@EhlOps EhlOps commented Mar 26, 2025

Changes

Added a int8_t queue_prio_can_msg(can_msg_t msg)

Notes

This function is NOT dependent upon queue_and_set_flags, but contains the same code. If we wanted to add a priority param to queue_and_set_flags, we could consolidate code. We could also add a priority param to queue_can_message, but I would advise against it as queue_can_message is already significantly used and we only want to distinguish between "priority" and "not piority"

Test Cases

  • Queue a regular message and a priority message, then receive
  • Queue a priority message and a regular message, then receive
  • Queue 100 regular messages and 10 priority messages, then receive
  • Queue 2 regular messages and receive
  • Queue 2 priority messages and receive

To Do

  • Change queue_and_set_flags in Embedded Base (optional)
  • Actually integrate use of queue_prio_can_message

Checklist

It can be helpful to check the Checks and Files changed tabs.
Please reach out to your Project Lead if anything is unclear.
Please request reviewers and ping on slack only after you've gone through this whole checklist.

  • No merge conflicts
  • All checks passing
  • Remove any non-applicable sections of this template
  • Assign the PR to yourself
  • Request reviewers & ping on Slack
  • PR is linked to the ticket (fill in the closes line below)

Closes #282

@EhlOps EhlOps requested a review from jr1221 March 26, 2025 18:35
@jr1221 jr1221 requested a review from caiodasilva2005 March 26, 2025 19:00
Copy link
Contributor

@caiodasilva2005 caiodasilva2005 left a comment

Choose a reason for hiding this comment

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

Great work on this. I def think we should add a priority param on queue_and_set flags for this, but yes that would affect anything else that uses it. Gonna approve, but gonna wait to merge

Copy link
Contributor

@jr1221 jr1221 left a comment

Choose a reason for hiding this comment

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

please test then merge.

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.

CAN Message Priorities

4 participants