-
-
Notifications
You must be signed in to change notification settings - Fork 31
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
The ability to set the priority of sending messages has been implemented #278
Conversation
…e transmit priority have been added
…essage transmit priority have been added
…essage transmit priority have been added
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #278 +/- ##
=======================================
Coverage 31.56% 31.56%
=======================================
Files 8 8
Lines 5028 5028
Branches 542 542
=======================================
Hits 1587 1587
Misses 3366 3366
Partials 75 75 ☔ View full report in Codecov by Sentry. |
Hi @aentinger. Due to the force push in my fork, I have to reopen the pull request. Message priority is now set via overloaded constructors instead of default values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ciao @bakul14 ☕ 👋 - this is going to the right direction, but adding your functionality created code duplication that now needs to be cleaned up.
@@ -43,7 +50,24 @@ Publisher<T> Node::create_publisher(CanardPortID const port_id, CanardMicrosecon | |||
return std::make_shared<impl::Publisher<T>>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the more configurable create_publisher
method below this is basically unnecessary code duplication. I suggest removing the body of this function and replacing it with
return create_publisher<T>(port_id, tx_timeout_usec, CanardPriorityNominal);
@@ -102,6 +135,36 @@ ServiceServer Node::create_service_server(CanardPortID const request_port_id, Ca | |||
*this, | |||
request_port_id, | |||
tx_timeout_usec, | |||
CanardPriorityNominal, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as for create_publisher, this is a lot of code duplication now. Replace with
return create_service_server<T_REQ, T_RSP, OnRequestCb>(request_port_id, tx_timeout_usec, on_request_cb, CanardPriorityNominal. tid_timeout_usec); // Probably needs std::forward for on_request_cb
std::forward<OnResponseCb>(on_response_cb) | ||
); | ||
|
||
int8_t const rc = canardRxSubscribe(&_canard_hdl, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same. eliminate code duplication.
Ciao @bakul14 ☕ 👋 I am awaiting your reply on my above comments, should there be none I see myself forced to close this PR. |
The priority of sending messages is set by the constructor argument when allocating the appropriate publishers, clients, servers. Thus, content will be sent with a predefined priority, which cannot be changed at runtime.