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

#67 Functions for unpacking based on byte arrays #71

Merged
merged 14 commits into from
Aug 27, 2024

Conversation

Uight
Copy link
Contributor

@Uight Uight commented Aug 10, 2024

This adds some new functions for packing/unpacking can messages based on byte arrays and does also support message lengths != 64 bit.

For me this is considered a base implementation that could be used to build on. (E.g. Message encode/decode function)

I also added a new benchmark project to the solution using dotnetbenchmark. This is optional but could help to tune performance in the future.

resolves #67 (but could be extended in the future to implement message packing aso.)

@EFeru
Copy link
Owner

EFeru commented Aug 19, 2024

Hi @Uight ,
Thanks for the PR. You said there was something strange with the GetStartBitLE function. Did you manage to figure it out?
I am in parallel checking another implementation in js:

https://github.com/bit-dream/candied/blob/4e4077131d4104f417e750f916edae5cd9e76eb1/src/can/Can.ts#L167-L183

https://github.com/bit-dream/candied/blob/4e4077131d4104f417e750f916edae5cd9e76eb1/src/can/BitUtils.ts#L90-L102

@Uight
Copy link
Contributor Author

Uight commented Aug 20, 2024

@EFeru i figured that out i was just being stupid ;)
i now did check everything against the python cantools as described in the issue and everything seems to be the same. Assuming its done right in the can tools its also done correctly here.
If this PR goes through it would probably still be usefull to discuss how the final packer interface should look like in the end and do some more stuff there

@EFeru
Copy link
Owner

EFeru commented Aug 26, 2024

@Uight still need to look into this. I did not forget :)

Copy link
Collaborator

@Adhara3 Adhara3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, just a minor comment.
Moreover some review has already been erformed with @EFeru and I see many UT, so we are ok.

Thanks

{
var startBit = signal.StartBit;
var message = receiveMessage;

if (!signal.Intel())
{
var tempArray = new byte[message.Length];
var tempArray = new uint8_T[message.Length];
Array.Copy(message, tempArray, message.Length);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is bittersweet. It's easy and readable but allocating to just read backward it's a pity.
Maybe if we change the ExtractBits signature into something like

private static uint64_T ExtractBits(uint8_T[] data, int startBit, int endBit, int step = 1) // Or -1 if backward

we could keep the same iteration without assuming that length is the limit. Just food for thoughts.

...or... (this is a bit outside the scope of this PR, but worth mentioning)
Today the Packer is a static class so it needs Signal and message info. If we optimize the Signal class (or if we adda a sort of .GetReader() on it) then we could optimize some stuff, removing ifs, and give a forward/backward reader depending on endianness

@Adhara3 Adhara3 merged commit 5df13b0 into EFeru:main Aug 27, 2024
1 check passed
@Uight Uight deleted the Functions-for-unpacking-based-on-byte-arrays branch August 27, 2024 14:59
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

Successfully merging this pull request may close these issues.

Unpack signal for CAN FD messages
3 participants