search

arduino / ArduinoCore-API

Hardware independent layer of the Arduino cores defining the official API
https://www.arduino.cc/reference/en/
GNU Lesser General Public License v2.1
216 stars 120 forks source link

Extend CanMsg to allow to distinguish between standard and extended ids … #194

Closed aentinger closed 1 year ago

aentinger commented 1 year ago

… following a <linux/can.h> inspired approach.

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.16% :tada:

Comparison is base (84b98c7) 95.77% compared to head (e091080) 95.93%.

Additional details and impacted files ```diff @@ 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 ``` | [Files Changed](https://app.codecov.io/gh/arduino/ArduinoCore-API/pull/194?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=arduino) | Coverage Δ | | |---|---|---| | [api/CanMsg.h](https://app.codecov.io/gh/arduino/ArduinoCore-API/pull/194?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=arduino#diff-YXBpL0Nhbk1zZy5o) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

aentinger commented 1 year ago

Hi @facchinm :coffee: :wave:

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 :100: 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.

facchinm commented 1 year ago

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.