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

Fix boolean convenience setters for ACF-CAN #67

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SebastianSchildt
Copy link
Collaborator

This fixes #66 for ACF-CAN.

Choice of input datatypes:

  • mtv: 64 bit, this is the save choice and the underlying functions are 64 bit anyway
  • rtr/eff: these are related to a can id, which at most is 29 bit, and mostly the flags are included with, e.g. in can_frame in Linux wich is 32 bit
  • FD related flags: BRS/FDF: 8 bit input. in Linux those are in an uint8 field in the canfd_frame struct

@adriaan-niess
Copy link
Member

adriaan-niess commented Jan 3, 2025

Hi Sebastian, thanks for all the help and effort. This is very much appreciated. But I do have some questions on this PR. Maybe we can discuss this next week when I'm back in office?

  • MTV (aka message timestamp valid) is a 1bit flag in IEEE1722 PDUs. Why should it be changed to 64bit? I've choosen uint8 because it seemed to be the smallest and most reasonable datatype (also thought about the _Bool type from stdbool.h, but decided against because I didn't know how portable this would be with respect to compilers used for automotive)
  • You've added if-conditions to the setters to set '1' in case of all values that evaluate to true. Atm the values are only matched against a bit mask, so setting a flag to '2' would actually set it to '0' since only the LSB would be read and other bits truncated. You're approach makes sense to me, however if we go with your approach we should try to keep the overall API consinstent for all flags (eventually). Not just for the flags defined in ACF-CAN.
  • The RTR/EFF flags are probably redundant since the information is also encoded into the Can ID. Nonetheless this is how it's defined in the IEEE1722 spec. I'm sceptical if it makes sense to closely align the API for accessing the IEEE1722 PDUs with Linux APIs since these data formats are meant to be used as a canonical intermediate form to transfer data between different systems, not necessarily just Linux Systems. So a conversion between the IEEE1722 eff/rtr flags and the system specific rtr/eff flags (and other fields) is expected to happen at some point.

@SebastianSchildt
Copy link
Collaborator Author

I tried to set the length so that it can be "expected" to work with any size a user might want to throwe at it, when (as it is very likely for some fields in CAN, see example code in the issue) the values are given by maskig something.

I think it is always a bit of an arbitrary choice, a "safe" default would be always 64 bits (which is used in the more generic parts of the code anyway). I chose that for mtv as that is a "non-CAN related" 1722-specific thing, whereas for rtr/eff I assumed no matter the CAN impl (Linux or otherwise) it is highly likely the flags, if they are packed will reside in a 32 bit integer, but unkikely they would be in a (uper half of an) 64bit int.

Generally, after thinking /discussing about it, I fell maybe for "bools" (1 bit values) the answer is not discussing int-length in the setters, but rather providing "argument-less" (re)setters for this, like, e.g.

void Avtp_Can_EnableEff();
void Avtp_Can_DisableEff();

Then the semantics are clear, and no casting surprises.
If I find time I may modify this PR.

In the end, of course we should use similar patterns for other "ACF's" as well.

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.

1-bit convenience setters not working as expected (in CAN, potentially others)
2 participants