Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork130
Extend CanMsg to allow to distinguish between standard and extended ids …#194
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…Ds following a <linux/can.h> inspired approach.
codecov-commenter commentedAug 2, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@## master #194 +/- ##==========================================+ Coverage 95.77% 95.93% +0.16%========================================== Files 13 14 +1 Lines 970 1009 +39 ==========================================+ Hits 929 968 +39 Misses 41 41
☔ View full report in Codecov by Sentry. |
…n order to eliminate the 3 MSBits encoded status flags.
…ardId", "isExtendedId".
… files, inline functions to prevent linker errors, and define static constants so that the compiler can place them in memory. All of those changes broaden adoption.
…ds max data lenght.
Hi@facchinm ☕ 👋 This PR is required by
which support both standard (11-Bit) and extended (29-Bit) CAN IDs by the respective platform libraries. I'm not a 💯 on the proposed design, especially |
thirtytwobits left a comment
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.
Really exciting to see CAN move forward in Arduino and looking forward to CAN-FD support in this header.
| @@ -82,11 +92,45 @@ class CanMsg : public Printable | |||
| return n; | |||
| } | |||
| uint32_t getStandardId() const { | |||
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.
Would it make sense to make the ID an ADT? That way you could have something like:
class CanId : public Printable{ virtual uint32_t toInt() const = 0;};class ExtendedCanId : public CanId...class StandardCanId: public CanId...This would allow libraries to be written that extend this to things like:
class DroneCANId : public ExtendedCanId...class CyphalId : public ExtendedCandId...class ARINC825 : public ExtendedCanId...// etc.| @@ -31,14 +33,19 @@ namespace arduino | |||
| class CanMsg : public Printable | |||
| { | |||
| public: | |||
| static size_t constexpr MAX_DATA_LENGTH = 8; | |||
| static uint8_t constexpr MAX_DATA_LENGTH = 8; | |||
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.
Of course, the thing that is most irksome when you finally add CAN-FD support is the quantization of the DLC. If you are trying to get ahead of adoption and avoid future churn you might want to start moving away from an integer data_length type to an enumerated one now.
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.
I had been thinking of making aCanFdMsg whoseMAX_DATA_LENGTH would be 64 then. Overall I'm a fan ofconst/constexpr overenum because they provide a strong type guarantee (vs.enum that easily and without warning converts to int, etc.).
Note: enum classes in public Arduino APIs are discouraged (they'd provide strong type support).
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
I'm ok for leaving it public to keep the APIs space simpler and allow some optimizations for libraries adopting this. |
… following a <linux/can.h> inspired approach.