-
-
Notifications
You must be signed in to change notification settings - Fork 82
Add a wrapper for Zephyr RTOS #77
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
base: master
Are you sure you want to change the base?
Conversation
Make license information machine-readable by using SPDX comment. SPDX information allows tools like `west spdx` to collect license information when building software to track license of the result. Signed-off-by: Dmitrii Sharshakov <[email protected]>
Allows importing can2040 repo for use within a Zephyr RTOS application. Signed-off-by: Dmitrii Sharshakov <[email protected]>
Signed-off-by: Dmitrii Sharshakov <[email protected]>
By default, can2040 initializes pinctrl, clock and resets as needed for operation. However, in some cases like using within an RTOS with centralized device management subsystems, this is not desired and might hinder debugging. Break out some functions to make sure one can configure everything outside and can2040 only works with the chosen PIO block. Signed-off-by: Dmitrii Sharshakov <[email protected]>
For some cases like an RTOS you might want to have some data apart from the can2040 internal state. This field can be set to some data, like an RTOS' device instance, and this data can be accessed from the can2040 instance passed to the callback. Signed-off-by: Dmitrii Sharshakov <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a draft for the time being since I do not really know how the integration into Zephyr will go, and maybe it'll need some more changes.
For now though I consider this pretty much what is needed, so I kindly request a review from you @KevinOConnor, we can discuss this change whenever you have time.
Thanks!
@@ -3,6 +3,7 @@ | |||
// Copyright (C) 2022-2025 Kevin O'Connor <[email protected]> | |||
// | |||
// This file may be distributed under the terms of the GNU GPLv3 license. | |||
// SPDX-License-Identifier: GPL-3.0-only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I correct to assume it's GPL-3.0-only, and not -and-later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zephyr build system West uses SPDX IDs to keep track of license of each file compiled, so this is important to make sure whoever uses this driver in Zephyr (or as my out-of-tree integration package if upstreaming fails) can get your licensing information and stay compliant with it.
Glue code must be Apache for Zephyr codebase, but when linking build system will find SPDX tags and let the user know they use a GPL-licensed module, fetched from a separate repository at the user's decision.
|
||
// Zephyr already defines these helpers | ||
// Avoid redefinition warnings | ||
#ifndef __ZEPHYR__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate your platform-agnostic approach, so I can make these macros individually ifndef-guarded to not introduce a Zephyr-specific thing
void can2040_ll_data_state_clear_bits(struct can2040 *cd); | ||
void can2040_ll_data_state_go_discard(struct can2040 *cd); | ||
void can2040_ll_pio_set_clkdiv(struct can2040 *cd, uint32_t sys_clock, uint32_t bitrate); | ||
void can2040_ll_pio_sm_setup(struct can2040 *cd); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A different interfix than _ll_
can be thought of. Feel free to suggest, this was just first off the top of my head. These might also go unseparated from the "main" API if you wish
Interesting project, but as discussed on Discord, I am not sure the GPLv3 license will acceptable in upstream Zephyr (which is Apache-2.0) - even as a module. |
Yes, I made it a draft due to this doubt. Anyway these changes might be useful even for an out-of-tree usage. |
Done making this an out-of-tree module. CANnectivity can now be built as easy as this + a bit of an overlay: Detailswest.yml: # Copyright (c) 2024-2025 Henrik Brix Andersen <[email protected]>
# SPDX-License-Identifier: Apache-2.0
manifest:
remotes:
- name: dsseng
url-base: https://github.com/dsseng
projects:
- name: zephyr
remote: dsseng
revision: dss-worktree
clone-depth: 1
import:
name-allowlist:
- cmsis_6
- hal_rpi_pico
- name: can2040
url: https://github.com/dsseng/can2040.git
revision: zephyr rpi_pico2_rp2350a_m33.overlay:
|
This commit adds necessary metadata and glue code for including can2040 driver as an out-of-tree module for Zephyr projects. Files of my work and those derived from Zephyr source are dual-licensed, which has no implications on can2040.c licensing. I license the glue code permissively to allow easier reuse for writing other CAN drivers Signed-off-by: Dmitrii Sharshakov <[email protected]>
Interesting. Thanks. I'm glad you've been able to have success with can2040 and that you've been able to get it to work with Zephyr. I have a handful of comments - in no particular order. I'm not really prepared to maintain build rules for different integrations. So, I think you may need to maintain the Zephyr support in your own repo, externally from this code repository. Either by copying the code (and attribution) or by using build rules to automatically pull in the code from this repo. When I started this code, I was aware that some users would want to use the RPi SDK, some would want to use FreeRTOS, some would want to use Zerphr, some would want to use "bare metal", some would want to use the arduino wrappers, some would want to use micropython, etc. So, I tried hard to avoid being dependent on any of them and to only use basic C code. I feel I'm not in a good position to be a maintainer of these per-platform build targets (and definitely not all of them). I'm a little surprised you needed to modify the can2040.c and can2040.h files. For what it is worth, you may want to consider minimizing those changes - briefly:
struct my_can2040_wrapper {
struct can2040 can;
void *rx_cb_user_data;
};
static void can_can2040_callback(struct can2040 *can2040_dev, uint32_t notify, struct can2040_msg *msg)
{
struct my_can2020_wrapper *cw = CONTAINER_OF(can2040_dev, my_can2040_wrapper, can);
const struct device *dev = cw->rx_cb_user_data;
...
}
That's fine. If someone really wants "and later" we can discuss it. Cheers, |
Agreed. Actually I'll probably just leave the Zephyr driver and build system code in a separate branch, only upstreaming necessary changes to the core. This way I'd be maintaining the Zephyr integration myself. Of course no need for you to increase the maintenance burden.
Will take a look, maybe those headers could be disabled for can2040.c
I have not, will take a look. I did everything the idiomatic way as in nothing but PIO accessed by can2040, but if all the accesses are atomic and otherwise not causing trouble this could be the way, albeit a bit weird. Will check when I'm back to this project
Thank you for an idea, I have not considered this. Will try
Thanks for clarification |
Some changes made to ease working with RTOS platforms, including Zephyr.
You can take a look at my Zephyr integration effort here.
After I clean it up I would like to try upstreaming it into Zephyr to enable using can2040 external module. By the way, is it CAN2040 or can2040, should we capitalize?
Zephyr wrapper code is rather unfinished and will be improved upon in next weeks.
docs: add SPDX headers to library sources
Make license information machine-readable by using SPDX comment.
SPDX information allows tools like
west spdx
to collect licenseinformation when building software to track license of the result.
build: add Zephyr module definition
Allows importing can2040 repo for use within a Zephyr RTOS application.
can2040: fix redefinitions when used with Zephyr
can2040_ll: add a low-level interface
By default, can2040 initializes pinctrl, clock and resets as needed for
operation.
However, in some cases like using within an RTOS with centralized
device management subsystems, this is not desired and might hinder
debugging. Break out some functions to make sure one can configure
everything outside and can2040 only works with the chosen PIO block.
can2040: add a user data field
For some cases like an RTOS you might want to have some data apart from
the can2040 internal state.
This field can be set to some data, like an RTOS' device instance,
and this data can be accessed from the can2040 instance passed to the
callback.
zephyr: add Zephyr driver integration
This commit adds necessary metadata and glue code for including can2040
driver as an out-of-tree module for Zephyr projects.
Files of my work and those derived from Zephyr source are dual-licensed,
which has no implications on can2040.c licensing. I license the glue
code permissively to allow easier reuse for writing other CAN drivers