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

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

Open
SebastianSchildt opened this issue Jan 2, 2025 · 0 comments · May be fixed by #67
Open

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

SebastianSchildt opened this issue Jan 2, 2025 · 0 comments · May be fixed by #67

Comments

@SebastianSchildt
Copy link
Collaborator

ACF-CAN offers convenience funtions to set fields, e.g.

Open1722/src/avtp/acf/Can.c

Lines 166 to 169 in 7fe06a3

void Avtp_Can_SetEff(Avtp_Can_t* pdu, uint8_t value)
{
SET_FIELD(AVTP_CAN_FIELD_EFF, value);
}

This works fine for all fiedls with "int" semantics, but does not behave as expected for 1 bit fields with bool semantic.

In C you might do

    Avtp_Can_SetEff(&pdu.can, cf->can_id & CAN_EFF_FLAG);

where according to C sematics cf->can_id & CAN_EFF_FLAG evaluates to true for extended frame identifiers, however as CAN_EFF_FLAG is defined as

#define CAN_EFF_FLAG 0x80000000U /* EFF/SFF is set in the MSB */

the value is not set.

I would suggest to change the convenicence functions for 1 Bit Ids, e.g. to

void Avtp_Can_SetEff(Avtp_Can_t* pdu, uint64_t value)
{
    if (value) {
        SET_FIELD(AVTP_CAN_FIELD_EFF, 1);
    } else {
        SET_FIELD(AVTP_CAN_FIELD_EFF, 0);
    }
}

The generic Avtp_Can_SetField and the even more generic Avtp_SetField(Avtp_CanFieldDesc, AVTP_CAN_FIELD_MAX, (uint8_t*)pdu, field, value). exhibit the same behavior, but I would not overcomplicate those with "special" boolean handling (they would need to have special handling depending on field description), but only choose the highest level direct access convenience functions as in the example

@SebastianSchildt SebastianSchildt linked a pull request Jan 2, 2025 that will close this issue
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 a pull request may close this issue.

1 participant