-
-
Notifications
You must be signed in to change notification settings - Fork 133
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
...Ds following a <linux/can.h> inspired approach.
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
- [Arduino_CAN] support both 11-Bit (standard) and 29-Bit (extended) IDs. ArduinoCore-renesas#79
- [Arduino_CAN] support both 11-Bit (standard) and 29-Bit (extended) IDs ArduinoCore-mbed#709
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 id still being a publicly exposed member variable. Personally I'd like to turn id private, something that would also work with both aforementioned pull requests, but may cause problems for users which have already adopted the new CAN API. I'd love to hear your thoughts on this.
@thirtytwobits
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.
@thirtytwobits
thirtytwobits
Aug 10, 2023
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.
@thirtytwobits
thirtytwobits
Aug 10, 2023
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 a CanFdMsg whose MAX_DATA_LENGTH would be 64 then. Overall I'm a fan of const/constexpr over enum 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).
especially id still being a publicly exposed member variable.
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.