Adds flexible data rate CAN payload to account for 64 byte CANFD and > 64 byte ethernet payloads #21

jshiv · 2021-04-01T16:26:03Z

Thanks for the great package! We have added some functionality for decoding >8 byte signals. The primary backwards incompatible change is Message.Length, Signal.Start, Signal.Length type changed from uint8 -> uint16.

Adds payload.go with type Payload
- Payload.Data is flexable []byte
- *bit.Int to enable >uint64 packed bit arrays
Signal.UnmarshalPhysicalPayload method distinct from Signal.UnmarshalPhysical
- Maintains separate api for Data and Payload because Data decoding is more performant than Payload
Signal.Decode and Message.Decode methods
- Does not force decoded physical values to min/max of range
README.md decoding example
Unit tests.

* Updated Message.Length to uint16 to support message lengths up to 65,535 bytes * Updated Signal.Lenght and Signal.Size to uint16 for payloads larger than 8 bytes * Added payload.go and payload_tests.go * Updated Big and Little Endian encoding methods * Added benchmarks * Added methods and tests to decode signed/unsigned signals in big/little endian * Added benchmarks for UnsignedBitsLittleEndian * Added hex string methods for Payload * Added UnmarshalPhysicalPayload and updated bit-level methods for Payload struct * Added UnmarshalValueDescriptionPayload and associated methods for Payload data struct * Added decode_tests.go * Updated decode test * Added asserts to decode_test.go * Added comparitive unit tests for can.Data and benchmarks for decoding * Updated PackLittleEndian and PackBigEndian to return new big.Int and reference pointer to payload in decode methods * Fixed units in VDM_Disconnect test Co-authored-by: Jason Shiverick <[email protected]> Co-authored-by: Naveen Venkatesan <[email protected]> Co-authored-by: Naveen Venkatesan <[email protected]>

…ls (#2) Co-authored-by: Naveen Venkatesan <[email protected]>

….Data) (#3) Co-authored-by: Jason Shiverick <[email protected]>

…ifting ranges

Added Decode and DecodePayload signal methods for decoding without sh…

Added message.Decode method

…sage (#6) * Updated README with example of decoding and dbc file with >8 byte message * Fixed indenting in README * Fixed indenting in README part 2 * Updated filename to example_payload.dbc * Updated dbc messages and removed unnecessary code from payload.go Co-authored-by: Naveen Venkatesan <[email protected]>

…the README

odsod

Hi @jshiv, @venkatesannaveen and team Rivian!

Huge thanks for using can-go and proposing this contribution on adding CAN FD support. 🙌

I don't think that we should make breaking changes to the existing API for CAN FD. Have you considered adding a separate package canfd with separate Frame and Conn types?

We would love to work together on adding CAN FD support in a non-breaking way. We're just about leaving for Easter holiday now Swedish time - but let's look at how to go about this, and we look forward to working together to add support for this without breaking existing code.

odsod · 2021-04-01T17:36:54Z

It looks like you are focusing on the use case of decoding CAN payloads, but not necessarily reading/writing CAN FD frames via SocketCAN interactively, is that a correct assumption?

odsod · 2021-04-01T17:45:00Z

I see now that the breaking changes are focused on the descriptors and not the actual runtime API, so it might be workable after all. Please give us some time to review further, and to assess the impact of the changes. 🔍

jshiv · 2021-04-01T18:23:36Z

Hi @jshiv, @venkatesannaveen and team Rivian!

Huge thanks for using can-go and proposing this contribution on adding CAN FD support. 🙌

I don't think that we should make breaking changes to the existing API for CAN FD. Have you considered adding a separate package canfd with separate Frame and Conn types?

We would love to work together on adding CAN FD support in a non-breaking way. We're just about leaving for Easter holiday now Swedish time - but let's look at how to go about this, and we look forward to working together to add support for this without breaking existing code.

@odsod We will be happy to work with you to make this work! There is no time crunch for us, we mostly wanted to get a PR in to let you know where we are.

jshiv · 2021-04-01T18:24:44Z

It looks like you are focusing on the use case of decoding CAN payloads, but not necessarily reading/writing CAN FD frames via SocketCAN interactively, is that a correct assumption?

Yes we have primarily been focused on decoding CAN payloads and have not used the SocketCAN functionality. This package has been great BTW!

alethenorio · 2021-04-01T18:38:07Z

Loving the community contribution here. Let's make sure to work together to get this merged.

Thank you so much guys

…igned errors

jshiv · 2022-07-22T19:00:54Z

Hi @odsod, Its been a while on this one, Just wanted to ping and see if there are has been any though on merging this?

Removed copy from reverse function

Feature/add multiplex decoding

hashemmm96 · 2022-11-18T12:33:20Z

Hi @jshiv! I'm going to be maintaining this repo going forward and wanted to check with you what the status is of this PR. We really appreciate the effort you put into this so far! Perhaps we can start by rebasing on latest master and cleaning up the commits. Then I can review and we'll see how to merge this properly.

jshiv · 2022-11-18T17:52:09Z

Hi @hashemmm96! Thanks for looking at it! We will work on rebasing.

Co-authored-by: Jason Shiverick <[email protected]>

hashemmm96

Nice work, and thanks for the contribution. Sorry for taking so long for reviewing.

A question regarding the payload.go. Are you using it to decode arbitrary payloads, or only CAN/CAN FD? The reason I'm asking is due to the PR name mentioning ethernet.

See the comments I left on the uint8/uint16 topic. Changing to uint16 makes sense for the Signal type, where we are counting bits, but not for the types where we're counting bytes.

Also note that the CI now formats code and checks that commits are following the "conventional commits" specification, so there's some cleaning up required there.

hashemmm96 · 2022-12-07T10:25:45Z

frame.go

 	// ID is the CAN ID
 	ID uint32
 	// Length is the number of bytes of data in the frame.
-	Length uint8
+	Length uint16
 	// Data is the frame data.
 	Data Data
 	// IsRemote is true for remote frames.


In CAN FD the DLC field may be 16 bits but in our case we're only counting the number of bytes in a frame. The Frame struct is supposed to parse candump output, which shows the length of the frame in bytes (maximum of 64 bytes for CAN FD) and as such there is no need to modify the Length field to uint16.

Something that needs to be changed instead is the Data type, defined in data.go. There it is hard coded as a slice of length 8, but should be increased to 64 instead.

We use an ethernet protocol that can actually send messages >255 bytes in length so the uint8 wasn't sufficient to hold the message length, which is why we changed this - open to suggestions on how you'd want to deal with this

Your ethernet protocol must be some kind of higher-layer protocol which utilizes CAN in the link layer. In that case, I think the best option would be that you implement a new library for your higher-layer protocol which depends on our library. There, you can wrap the Frame type in your own structs to fit your use case. As I see it this is a pure CAN library, thus we should limit the scope to the link-layer aspects and DBC file handling.

I assume you're using the descriptor library to parse signals within your protocol, meaning your signals can be more than 255 bits wide as well. This is something we can implement in this library, since signals don't really follow a standard size structure the same way the CAN and CAN FD protocols do. That will be a breaking change but would make sense to do if we're implementing CAN FD support.

Correct, we actually may be able to extract the message length from the header in our ethernet protocol so we can potentially revert this back to a uint8 - the main issue with > 8 bytes (CAN-FD or arbitrary length) is that we're no longer able to use uint64 to pack the bits so we've resorted to using math.big to handle this which by nature would allows lengths of any size. We can refactor some of our decoding code to deal with a 64 byte payload, it just seems like a more limiting constraint. We see value in keeping the current can.Data struct and associated methods since it's very efficient for messages <= 8 bytes, and we currently use both.

Would your team be open to a meeting on Zoom, etc. to talk through some of our design choices? This may be much simpler than going back-and-forth on the comments here. @jshiv and I are both in PST which is 9 hours behind Sweden but we can work something out if that sounds good on your side.

I think that's a great idea to talk "face-to-face" 😄

I sent you a request on LinkedIn, let's exchange contact info there.

hashemmm96 · 2022-12-07T10:46:55Z

tests/decode_test.go

@@ -0,0 +1,415 @@
+package decode_test


This test should probably be moved up a dir and renamed to message_test.go

hashemmm96 · 2022-12-07T10:58:14Z

pkg/socketcan/frame.go

 	// idAndFlags is the combined CAN ID and flags.
 	idAndFlags uint32
 	// dataLengthCode is the frame payload length in bytes.
-	dataLengthCode uint8
+	dataLengthCode uint16
 	// padding+reserved fields
 	_ [3]byte
 	// bytes contains the frame payload.


In the SocketCAN API, DLC is deprecated. Instead this field refers to the payload length in bytes, and is a uint8.

What actually needs to be changed is the data field in the frame struct. It has a fixed size of 8 bytes and should be increased to 64 to allow for CAN FD payloads. However, I think we should wait with adding CAN FD support to the socketcan package until we also have support for setting up/down CAN FD devices.

naveenv92 · 2022-12-07T18:37:16Z

A question regarding the payload.go. Are you using it to decode arbitrary payloads, or only CAN/CAN FD? The reason I'm asking is due to the PR name mentioning ethernet.

Our payload should support arbitrarily long messages so the CAN/CAN-FD is not relevant anymore so we can change the title of the PR to reflect that

RangelReale · 2023-12-19T20:14:49Z

I'm also receiving payloads larger than 8 bytes (16 and 64).

They are parsed by python cantools without any problem, maybe we can just check if each signal start/length is inside the payload size and not limit it to 8 bytes?

jshiv and others added 14 commits March 1, 2021 15:45

Assign byte array directly to big.Int for big and little endian signa…

047b82f

…ls (#2) Co-authored-by: Naveen Venkatesan <[email protected]>

Removed payload.Length property and calculate length with len(payload…

9986b66

….Data) (#3) Co-authored-by: Jason Shiverick <[email protected]>

Added Decode and DecodePayload signal methods for decoding without sh…

b87d69e

…ifting ranges

Added unit tests for Decode() to signal_test.go

592beb4

Added unit tests to test Decode() and DecodePayload()

fd0c28c

Added unit tests for UnmarshalPhysical() and UnmarshalPhysicalPayload()

f845a4a

Merge pull request #4 from jshiv/fix/do_not_shift_ranges_signal_decode

51c1b50

Added Decode and DecodePayload signal methods for decoding without sh…

Added message.Decode method

36ce676

Updated message.Decode to use signal.Decode internally

cdee1a2

Merge pull request #5 from jshiv/feature/message_decode

6e5dbc8

Added message.Decode method

Merge remote-tracking branch 'upstream/master'

ec1debd

Merged with upstream/master

a2d4186

jshiv requested review from alethenorio, odsod and oll3 as code owners April 1, 2021 16:26

Naveen Venkatesan added 2 commits April 1, 2021 09:51

Removed example_payload.dbc and added a string dbc to the example in …

ef9ceb3

…the README

Shortened DBC in the README and fixed linting issues

0248a67

odsod requested changes Apr 1, 2021

View reviewed changes

Jason Shiverick and others added 5 commits April 1, 2021 11:57

Removed global for linter

f697a62

Fixed linting issues - passes all golint tests locally

16b04f8

go mod tidy

0ec95e8

Updated generated go code from new Message struct format to avoid mal…

b67ca94

…igned errors

Merge branch 'master' of github.com:jshiv/can-go

623b114

Naveen Venkatesan added 3 commits July 21, 2022 19:23

Added multiplexing

70928fe

Updated README

ba99d84

Updated README

24b187a

Naveen Venkatesan and others added 9 commits August 2, 2022 13:53

Fixed reverse function

f5e2511

Merge remote-tracking branch 'upstream/master'

2376779

Removed copy from reverse function

d599b93

Cleaned up code

f5d4371

Reverted to len(data) from len(reversedArray)

2a78a25

Changed reverse to append logic and updated unit test

68bf611

Merge pull request #9 from jshiv/fix/change_reverse

615c5a9

Removed copy from reverse function

Merge branch 'master' of github.com:jshiv/can-go

a76019b

Merge pull request #8 from jshiv/feature/add_multiplex_decoding

4d6ffd9

Feature/add multiplex decoding

Merge branch 'einride:master' into master

7487488

jshiv requested a review from a team November 18, 2022 20:28

Fixed broken UnmarshalValueDescriptionPayload (#10)

566b73c

Co-authored-by: Jason Shiverick <[email protected]>

hashemmm96 suggested changes Dec 7, 2022

View reviewed changes

odsod requested review from a team and removed request for oll3, odsod and alethenorio February 28, 2024 07:36

Jassob requested review from Jassob and removed request for a team May 16, 2024 15:44

Jassob added api API changes codegen Changes to the generated code labels May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds flexible data rate CAN payload to account for 64 byte CANFD and > 64 byte ethernet payloads #21

Adds flexible data rate CAN payload to account for 64 byte CANFD and > 64 byte ethernet payloads #21

jshiv commented Apr 1, 2021 •

edited

Loading

odsod left a comment

odsod commented Apr 1, 2021

odsod commented Apr 1, 2021

jshiv commented Apr 1, 2021

jshiv commented Apr 1, 2021

alethenorio commented Apr 1, 2021

jshiv commented Jul 22, 2022

hashemmm96 commented Nov 18, 2022

jshiv commented Nov 18, 2022

hashemmm96 left a comment

hashemmm96 Dec 7, 2022

naveenv92 Dec 7, 2022

hashemmm96 Dec 8, 2022 •

edited

Loading

naveenv92 Dec 8, 2022

hashemmm96 Dec 9, 2022

hashemmm96 Dec 7, 2022

hashemmm96 Dec 7, 2022

naveenv92 commented Dec 7, 2022

RangelReale commented Dec 19, 2023

Adds flexible data rate CAN payload to account for 64 byte CANFD and > 64 byte ethernet payloads #21

Are you sure you want to change the base?

Adds flexible data rate CAN payload to account for 64 byte CANFD and > 64 byte ethernet payloads #21

Conversation

jshiv commented Apr 1, 2021 • edited Loading

odsod left a comment

Choose a reason for hiding this comment

odsod commented Apr 1, 2021

odsod commented Apr 1, 2021

jshiv commented Apr 1, 2021

jshiv commented Apr 1, 2021

alethenorio commented Apr 1, 2021

jshiv commented Jul 22, 2022

hashemmm96 commented Nov 18, 2022

jshiv commented Nov 18, 2022

hashemmm96 left a comment

Choose a reason for hiding this comment

hashemmm96 Dec 7, 2022

Choose a reason for hiding this comment

naveenv92 Dec 7, 2022

Choose a reason for hiding this comment

hashemmm96 Dec 8, 2022 • edited Loading

Choose a reason for hiding this comment

naveenv92 Dec 8, 2022

Choose a reason for hiding this comment

hashemmm96 Dec 9, 2022

Choose a reason for hiding this comment

hashemmm96 Dec 7, 2022

Choose a reason for hiding this comment

hashemmm96 Dec 7, 2022

Choose a reason for hiding this comment

naveenv92 commented Dec 7, 2022

RangelReale commented Dec 19, 2023

jshiv commented Apr 1, 2021 •

edited

Loading

hashemmm96 Dec 8, 2022 •

edited

Loading