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

CAN 2.0B Driver changes #69

Merged
merged 4 commits into from
Nov 7, 2024
Merged

CAN 2.0B Driver changes #69

merged 4 commits into from
Nov 7, 2024

Conversation

JasonBrave
Copy link
Member

@JasonBrave JasonBrave commented Sep 8, 2024

Update CAN driver only, update SID will be done in a separate PR

@JasonBrave JasonBrave changed the title CAN 2.0B CAN 2.0B Driver changes Sep 15, 2024
Copy link

@celery6 celery6 left a comment

Choose a reason for hiding this comment

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

can u add more comments for the bitshifting? make it more maintainable if someone else needs to debug it or anything.

Comment on lines +161 to +165
rcvd_msg.sid = (uint32_t)RXB1SIDH << 21;
rcvd_msg.sid |= (((uint32_t)RXB1SIDL >> 5) & 0x7) << 18;
rcvd_msg.sid |= ((uint32_t)RXB1SIDL & 0x3) << 16;
rcvd_msg.sid |= (uint32_t)RXB1EIDH << 8;
rcvd_msg.sid |= RXB1EIDL;
Copy link

Choose a reason for hiding this comment

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

add more comments so we know what this is doing at a glance?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can put a link to datasheet if you want?

Copy link

Choose a reason for hiding this comment

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

i was thinking smth like "store the [N:n] bits from rxb1eid into sid" or something like that to describe the bit shifting. unless its really easy to interpret these 5 bitwise ops and im just skill issuing?

Copy link
Contributor

Choose a reason for hiding this comment

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

adding datasheet page number is probably good enough

Copy link
Contributor

@AshTheEngiqueer AshTheEngiqueer left a comment

Choose a reason for hiding this comment

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

LGTM :)

@JasonBrave JasonBrave merged commit fa06983 into master Nov 7, 2024
@JasonBrave JasonBrave deleted the can-20b branch November 7, 2024 14:54
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.

4 participants