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

Expose len8_dlc field of can_frame struct. #3357

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

de-vri-es
Copy link
Contributor

@de-vri-es de-vri-es commented Sep 19, 2023

The can_frame struct on Linux has a len8_dlc field which can be used to stuff a few extra bits in the dlc field of the actual CAN frame on the wire (only allowed when the data length itself is 8, and if the hardware CAN controller allows it).

However, that field is not currently exposed. This PR exposes it as pub len8_dlc: u8 so we can get those extra 2.8 bits from our CAN frames 😅

It would have been nice to name the current can_dlc field as len, instead of the old deprecated name, but I guess that ship has sailed.

Upstream documentation: https://www.kernel.org/doc/html/latest/networking/can.html#how-to-use-socketcan

struct can_frame {
        canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
        union {
                /* CAN frame payload length in byte (0 .. CAN_MAX_DLEN)
                 * was previously named can_dlc so we need to carry that
                 * name for legacy support
                 */
                __u8 len;
                __u8 can_dlc; /* deprecated */
        };
        __u8    __pad;   /* padding */
        __u8    __res0;  /* reserved / padding */
        __u8    len8_dlc; /* optional DLC for 8 byte payload length (9 .. 15) */
        __u8    data[8] __attribute__((aligned(8)));
};

Remark: The len element contains the payload length in bytes and should be used instead of can_dlc. The deprecated can_dlc was misleadingly named as it always contained the plain payload length in bytes and not the so called 'data length code' (DLC).

To pass the raw DLC from/to a Classical CAN network device the len8_dlc element can contain values 9 .. 15 when the len element is 8 (the real payload length for all DLC values greater or equal to 8).

@rustbot
Copy link
Collaborator

rustbot commented Sep 19, 2023

r? @JohnTitor

(rustbot has picked a reviewer for you, use r? to override)

@de-vri-es de-vri-es changed the title Expose len8_dlc field of can_frame struct. Expose len8_dlc field of can_frame struct. Sep 19, 2023
@JohnTitor
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 21, 2023

📌 Commit 85dbb12 has been approved by JohnTitor

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 21, 2023

⌛ Testing commit 85dbb12 with merge 5c5c419...

bors added a commit that referenced this pull request Sep 21, 2023
Expose `len8_dlc` field of `can_frame` struct.

The `can_frame` struct on Linux has a `len8_dlc` field which can be used to stuff a few extra bits in the `dlc` field of the actual CAN frame on the wire (only allowed when the data length itself is 8, and if the hardware CAN controller allows it).

However, that field is not currently exposed. This PR exposes it as `pub len8_dlc: u8` so we can get those extra 2.8 bits from our CAN frames 😅

It would have been nice to name the current `can_dlc` field as `len`, instead of the old deprecated name, but I guess that ship has sailed.

Upstream documentation: https://www.kernel.org/doc/html/latest/networking/can.html#how-to-use-socketcan

> ```c
> struct can_frame {
>         canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
>         union {
>                 /* CAN frame payload length in byte (0 .. CAN_MAX_DLEN)
>                  * was previously named can_dlc so we need to carry that
>                  * name for legacy support
>                  */
>                 __u8 len;
>                 __u8 can_dlc; /* deprecated */
>         };
>         __u8    __pad;   /* padding */
>         __u8    __res0;  /* reserved / padding */
>         __u8    len8_dlc; /* optional DLC for 8 byte payload length (9 .. 15) */
>         __u8    data[8] __attribute__((aligned(8)));
> };
> ```
>
> Remark: The len element contains the payload length in bytes and should be used instead of can_dlc. The deprecated can_dlc was misleadingly named as it always contained the plain payload length in bytes and not the so called 'data length code' (DLC).
>
> To pass the raw DLC from/to a Classical CAN network device the len8_dlc element can contain values 9 .. 15 when the len element is 8 (the real payload length for all DLC values greater or equal to 8).
@bors
Copy link
Contributor

bors commented Sep 21, 2023

💔 Test failed - checks-actions

@de-vri-es
Copy link
Contributor Author

I don't see any reason for the failure. Is it correct to assume that this was an unrelated CI failure? :o

@JohnTitor
Copy link
Member

The failure is related. I think mips doesn't have it or our musl header is too old.

@de-vri-es
Copy link
Contributor Author

Oh, sorry, the link just pointed to a job running "exit 1" and I didn't notice the other red check.

@de-vri-es
Copy link
Contributor Author

de-vri-es commented Sep 30, 2023

Note that the can_frame struct comes from a Linux header, not from a glibc or musl header. I'm also pretty sure that mips has it too, but the headers are pulled in from the openwrt sdk which includes headers from linux 4.14, and the len8_dlc field was added in Linux commit ea7800565a128 which was released in 5.11.

What would be a good way forward for me to unblock this? Find a different linux mips distro with a newer kernel?

/edit: This openwrt release would fit the bill: https://downloads.openwrt.org/releases/23.05.0-rc4/targets/bcm47xx/generic/openwrt-sdk-23.05.0-rc4-bcm47xx-generic_gcc-12.3.0_musl.Linux-x86_64.tar.xz

@de-vri-es
Copy link
Contributor Author

Hmm, updating to https://downloads.openwrt.org/releases/23.05.0-rc4/targets/bcm47xx/generic/openwrt-sdk-23.05.0-rc4-bcm47xx-generic_gcc-12.3.0_musl.Linux-x86_64.tar.xz breaks other tests, atleast if I locally run ci/run-docker.sh mipsel-unknown-linux-musl.

I guess I need to figure out why first :(

@bors
Copy link
Contributor

bors commented Nov 11, 2023

☔ The latest upstream changes (presumably #3429) made this pull request unmergeable. Please resolve the merge conflicts.

@tgross35
Copy link
Contributor

@de-vri-es any chance you could update this? MIPS dropped to tier3 so if that was the cause of failure here, it shouldn't be a problem anymore.

@de-vri-es
Copy link
Contributor Author

Re-applied the changes to current main.

@de-vri-es
Copy link
Contributor Author

CI now fails because the kernel headers for the musl tests are taken from https://github.com/sabotage-linux/kernel-headers which is based on linux 4.19, but this field was introduced in linux 5.11 (released in february 2021).

@tgross35
Copy link
Contributor

Could you just update skip_field in libc-test/build.rs and leave a FIXME(musl)? We should be updating these in the near future

@de-vri-es de-vri-es force-pushed the can-frame-len8-dlc branch 2 times, most recently from f1e04fc to 987fd8d Compare November 25, 2024 09:11
@de-vri-es
Copy link
Contributor Author

Done, and rebased again!

@rustbot ready

@de-vri-es
Copy link
Contributor Author

Huh.. I don't see my changes to build.rs anymore in the git history but CI succeeded.. Maybe I messed up the rebase and someone updated the musl headers in the mean time? 😆

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -1583,7 +1583,7 @@ s_no_extra_traits! {
pub can_dlc: u8,
Copy link
Contributor

Choose a reason for hiding this comment

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

While you're here, mind adding a comment // FIXME(1.0): this field was renamed to `can_len` based on the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added!

@tgross35 tgross35 added the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Nov 25, 2024
@tgross35
Copy link
Contributor

Huh.. I don't see my changes to build.rs anymore in the git history but CI succeeded.. Maybe I messed up the rebase and someone updated the musl headers in the mean time? 😆

Indeed :) since #3921 musl is on 6.6

I left one quick unrelated request since you're updating the struct, should be ready in either case.

@de-vri-es
Copy link
Contributor Author

Indeed :) since #3921 musl is on 6.6

Nice :)

I left one quick unrelated request since you're updating the struct, should be ready in either case.

Added the comment to can_dlc (new name is "len") :)

@tgross35
Copy link
Contributor

Oops thanks for the name correction. LGTM!

auto-merge was automatically disabled November 25, 2024 11:08

Head branch was pushed to by a user without write access

@de-vri-es
Copy link
Contributor Author

Had to force-push again, I had a typo in FIXME.

tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Nov 25, 2024
@tgross35 tgross35 added this pull request to the merge queue Nov 25, 2024
Merged via the queue into rust-lang:main with commit 951cab5 Nov 25, 2024
45 checks passed
@de-vri-es
Copy link
Contributor Author

@tgross35: Thanks for the reviews!

@tgross35 tgross35 added stable-applied This PR has been cherry-picked to libc's stable release branch and removed stable-nominated This PR should be considered for cherry-pick to libc's stable release branch labels Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-linux O-unix S-waiting-on-ci S-waiting-on-review stable-applied This PR has been cherry-picked to libc's stable release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants