Skip to content
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

Merge CAN_FRAME and CAN_FRAME_FD #5

Open
driftregion opened this issue May 29, 2019 · 1 comment
Open

Merge CAN_FRAME and CAN_FRAME_FD #5

driftregion opened this issue May 29, 2019 · 1 comment

Comments

@driftregion
Copy link

It'd be elegant if there was a single CAN_FRAME class.
I think the largest reason against doing this is that a CAN_FRAME_FD structure requires about 4x more memory than a CAN_FRAME.

CAN_FRAME_FD is 64 + 12 + 5 = 81 bytes
CAN_FRAME is 8 + 12 + 4 = 24 bytes
The ESP32 has 520KiB of RAM, enough for about 6000 CAN_FRAME_FD or 20,000 CAN_FRAME.

Since most applications probably won't ever store more than about 100 frames in memory at once, it seems reasonable to merge these two classes.

I'm not using CAN FD at the moment, so there are probably plenty of things I haven't considered. What do you think about this idea?

@collin80
Copy link
Owner

Well, my biggest issue is that can_common runs across multiple processors. I use it for due_can on the Arduino Due (Atmel SAM3X) as well. There is only 96k of RAM there. Now, usually Arduino sketches don't allocate more than around 32 to 64 frames total in buffers. If we go with 64 then switching to the FD version takes up 64 * 57 = 3648 extra bytes for nothing. That still isn't huge even in 96k of RAM but perhaps not everyone wants to drop almost 4k of RAM for no purpose. Another oddity is that CAN-FD frames have a slightly different set of flags from CAN frames. The two structures in can_common do reflect that. But, I also do have functions that map between the two so I can always receive in CAN-FD and then turn those into CAN frames when needed.... Hmm, it would be simpler to be consistent but I'm not entirely sure whether it's a good idea or not. I'm keeping the issue open for now and more thought will have to go into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants