add acceptance filter configuration helper #171
Conversation
coderkalyan
force-pushed
the
acceptance-filters
branch
from
0b4d95d to
78400b0
Compare
There was a problem hiding this comment.
Wow this is nice. Few things to look into:
-
Please run
make formatto fix non-conformant formatting. You will need clang-format v11. -
Bump minor version & update the changelog.
-
The current test coverage of 0% is ∞ times below the required level.
-
I see that
canardMakeAcceptanceFilterConfigForService()accepts the service-ID as well, while in my proposal it was intentionally omitted. The assumption was that service transfers are addressed transfers rather than opt-in like messages. If you have a match on the destination node-ID, then there is a good chance you need to handle that transfer. Combined with the fact that in UAVCAN networks service traffic takes only a small percentage of the total network load, it makes sense to simplify the API by accepting all service transfers where the destination node-ID matches the local node-ID. The way you did it certainly also works but remember that the number of hardware acceptance filters is usually limited which will force most users to usecanardConsolidateAcceptanceFilterConfigs(). Both approaches are valid and usable, but if you prefer to keep yours, then I suggest updating the usage demo with a simple loop that consolidates all service filters into one config to show the most common usage scenario. Remember that you need not only service-IDs you serve but also those you invoke (otherwise responses will get lost). Overall this may somewhat increase the perceived complexity of the API.
Does it make more sense from an API perspective to use the CanardInstance itself as an argument for the service generator and pull the local node id from that?
Probably not, I think, because passing only the node-ID allows us to segregate the filter API from the rest of the library, potentially making this API more reusable (e.g., what if you need to configure filters for a different node). OTOH, if one were to turn this principle up to eleven, the entire CanardInstance would have to be refactored into a collection of smaller, more granular interfaces, which I would have probably done if this library was written in an OOP-friendly language instead of C. I think implementing it the way you did it is the most reasonable approach.
I just noticed that our CI is borked, asked Clyde to look into that.
|
Everything other than tests have been fixed (haven't pushed the updates yet). Still trying to figure out how/what tests to write 😄 The new Github Actions CI is also almost ready. |
|
Add a new file like |
|
@coderkalyan please rebase this to my v2 branch. |
coderkalyan
force-pushed
the
acceptance-filters
branch
from
9263a0c to
f7aa301
Compare
coderkalyan
force-pushed
the
acceptance-filters
branch
3 times, most recently
from
0b2eba7 to
5bf56cb
Compare
|
@pavel-kirienko This has been rebased onto your v2 branch, and I wrote a few lines of tests, but I'm not fully sure if they meet the requirements. Could you take a look and send feedback? |
There was a problem hiding this comment.
It generally looks okay with a few notes:
- The unit test won't compile, I take it it's still incomplete.
- Clang-Tidy complains about dangerous implicit casts, this one is an easy fix (just explicitly upcast your small fields like the node-ID).
- I neglected to notice it earlier but our newly added identifiers are longer than 31 character, which is non-conformant with AUTOSAR/MISRA. Can we please just replace
AcceptanceFilterConfig->Filtereverywhere (both functions and the type)?
coderkalyan
force-pushed
the
acceptance-filters
branch
6 times, most recently
from
0ea9197 to
79ad956
Compare
coderkalyan
force-pushed
the
acceptance-filters
branch
6 times, most recently
from
d3a7546 to
b6c1915
Compare
Adds helper API functions for configuring CAN controller hardware acceptance filters.
coderkalyan
force-pushed
the
acceptance-filters
branch
from
b6c1915 to
66633b4
Compare
|
Ready for review again. I believe all the comments have been addressed and the CI is passing now. Other than the sonarcloud, that is, which is addressed in #186 |
* Drop canard_dsdl, use Nunavut instead * Upgrade CI to LLVM 13 * Actualize license headers * Support redundant transmission queues and use more consistent public field naming * Ditch the deprecated canardRxAccept(), rename canardRxAccept2() * Refactor the API to eliminate the need to cast away const qualifiers; fixes #175 * Tighten up memory checking in the test suite -- add canaries * Fix race condition in the roundtrip test * Do not use Catch2 macros from non-main thread because it is not thread-safe * Support CANARD_CONFIG_HEADER * CI: add style_check job * Use AVL tree in the transmission queue * Remove all linked lists * Reduce indirection, pointer casts, and memory footprint by exposing the AVL tree in the public API * Disable C++-specific warnings as they make no sense for a C library * Add table-based CRC option (#186) * CI: disable SonarCloud on forks * Add docker utilities (#187) * Add acceptance filter configuration helpers (#171) Co-authored-by: Kalyan Sriram <[email protected]>
Briefly tested all 3 functions on a test project with yakut. Appears to work, and it's super simple.
@pavel-kirienko Does it make more sense from an API perspective to use the CanardInstance itself as an argument for the service generator and pull the local node id from that?
Closes #169