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

Minor can driver improvement #12078

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

xiaoxiang781216
Copy link
Contributor

Summary

  • can: Remove CANIOC_XXX macro from include/nuttx/can.h
  • can: Add g_ prefix to can_dlc_to_len and len_to_can_dlc
  • can: Merge cd_error and rx_overflow into rx_error

Impact

can driver

Testing

internal test case

to avoid the duplication in include/nuttx/can/can.h.

Signed-off-by: Xiang Xiao <[email protected]>
so the error could dispath to each client without interference

Signed-off-by: Xiang Xiao <[email protected]>
since routines called by IRQ need to be protected in SMP too

Signed-off-by: Xiang Xiao <[email protected]>
@acassis
Copy link
Contributor

acassis commented Apr 6, 2024

@xiaoxiang781216 could you please share your internal test? We need to confirm this modification will not break existing applications using CAN

@acassis
Copy link
Contributor

acassis commented Apr 7, 2024

@xiaoxiang781216 I think include/nuttx/can.h is legacy code, when I created the mcp2515 driver I needed to create a drivers/can and moved can.c to there, but I think this header file was left behind. Maybe we could merge it into include/nuttx/can/can.h

@xiaoxiang781216
Copy link
Contributor Author

xiaoxiang781216 commented Apr 7, 2024

@xiaoxiang781216 I think include/nuttx/can.h is legacy code, when I created the mcp2515 driver I needed to create a drivers/can and moved can.c to there, but I think this header file was left behind. Maybe we could merge it into include/nuttx/can/can.h

include/nuttx/can/can.h contains more ioctl than include/nuttx/can.h. This is the reason why I remove ioctl from include/nuttx/can.h.

@acassis
Copy link
Contributor

acassis commented Apr 7, 2024

@xiaoxiang781216 I think include/nuttx/can.h is legacy code, when I created the mcp2515 driver I needed to create a drivers/can and moved can.c to there, but I think this header file was left behind. Maybe we could merge it into include/nuttx/can/can.h

include/nuttx/can/can.h contains more ioctl than include/nuttx/can.h. This is the reason why I remove ioctl from include/nuttx/can.h.

Right, but maybe it makes more sense to have only include/nuttx/can/can.h

@xiaoxiang781216
Copy link
Contributor Author

Your mean merge include/nuttx/can.h into include/nuttx/can/can.h?

@acassis
Copy link
Contributor

acassis commented Apr 7, 2024

Your mean merge include/nuttx/can.h into include/nuttx/can/can.h?

Yes!

@pkarashchenko
Copy link
Contributor

Your mean merge include/nuttx/can.h into include/nuttx/can/can.h?

Yes!

That is not trivial. Having two can.h is needed to maintain both CAN char driver and Socket can driver. I've been looking there a while ago and didn't find an easy solution.

@acassis
Copy link
Contributor

acassis commented Apr 7, 2024

Your mean merge include/nuttx/can.h into include/nuttx/can/can.h?

Yes!

That is not trivial. Having two can.h is needed to maintain both CAN char driver and Socket can driver. I've been looking there a while ago and didn't find an easy solution.

Maybe we could separate it in include/nuttx/can/can.h and include/nuttx/can/socketcan.h since technically they are different

@xiaoxiang781216
Copy link
Contributor Author

include/nuttx/can.h follow linux style(include/uapi/linux/can.h). Linux remove the support of CAN char driver, so it isn't a problem to short SocketCAN to CAN.

@acassis
Copy link
Contributor

acassis commented Apr 8, 2024

include/nuttx/can.h follow linux style(include/uapi/linux/can.h). Linux remove the support of CAN char driver, so it isn't a problem to short SocketCAN to CAN.

Right, but for NuttX it is important to keep support for CAN char driver because some boards don't have enough memory to support SocketCAN.

@PetervdPerk-NXP
Copy link
Contributor

I rather keep the Char Device CAN and SocketCAN separate, they're both fundamentally different subsystems with their own API's.

@acassis
Copy link
Contributor

acassis commented Apr 10, 2024

I rather keep the Char Device CAN and SocketCAN separate, they're both fundamentally different subsystems with their own API's.

Agree!

@xiaoxiang781216
Copy link
Contributor Author

Yes, I would suggest that we write a general net driver on top of can driver, so the user just need write char driver and expose char or socketcan interface in defconfig.

@PetervdPerk-NXP
Copy link
Contributor

That would incur a lot of overhead (Zephyr is doing it and their SocketCAN performance isn't that great due to it) It isn't hard to support both back-ends for a can driver I did it for the LPC17XX.
https://github.com/apache/nuttx/blob/master/arch/arm/src/lpc17xx_40xx/lpc17_40_can.c

@xiaoxiang781216 xiaoxiang781216 marked this pull request as draft April 24, 2024 01:46
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.

None yet

4 participants