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.

Sign up for GitHub

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

Jump to bottom

ATSAM CAN driver (MCAN peripheral) #955

Merged
merged 9 commits into from
May 9, 2023
Merged

ATSAM CAN driver (MCAN peripheral) #955

merged 9 commits into from
May 9, 2023

Conversation

rleh
Copy link
Member

@rleh rleh commented Feb 5, 2023 •
edited

  • Porting of ST's FDCAN periphal driver (based on the same 3rd party intellectual property)
  • Use of existing the message_ram.hpp implementation
  • Add CAN pinout to SAMV71-XPLAINED board module (there is a CAN transceiver on the dev board!)
  • Adapt connect<...>()
  • Update HAL matrix
  • Long frame and fast frame support
  • Remove software-buffers and make hardware-buffers directly accessible and configurable.
  • Test on real hardware

@rleh rleh added this to the 2023q1 milestone Feb 5, 2023
@rleh

This comment was marked as resolved.

Sign in to view
@chris-durand

This comment was marked as resolved.

Sign in to view
@rleh
Copy link
Member Author

rleh commented Feb 6, 2023

  • Test on real hardware

It does not work yet; I suspect the clock configuration...

@rleh rleh marked this pull request as ready for review February 9, 2023 19:07
@rleh rleh added ci:hal Triggers the exhaustive HAL compile CI jobs and removed ci:hal Triggers the exhaustive HAL compile CI jobs labels Feb 9, 2023
salkinium
salkinium requested changes Feb 9, 2023
View reviewed changes
Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

Very nice and clean, only some minor notes.

examples/samv71_xplained_ultra/can/main.cpp Outdated Show resolved Hide resolved
src/modm/platform/can/atsam-fdcan/can.cpp.in Outdated Show resolved Hide resolved
src/modm/platform/clock/sam_pmc/clockgen.hpp.in Outdated Show resolved Hide resolved
@rleh rleh added ci:hal Triggers the exhaustive HAL compile CI jobs and removed ci:hal Triggers the exhaustive HAL compile CI jobs labels Feb 10, 2023
@rleh rleh marked this pull request as draft February 10, 2023 15:25
@rleh
Copy link
Member Author

rleh commented Feb 10, 2023

The Atmel variant of this CAN IP block has configurable buffers (total size up to 4352 words == 17KB) located in the "userspace" SRAM. (STM32 has only <1KB fixed message RAM and e.g. the Rx FIFOs only fit 3 elements each.)

The buffer element sizes are also configurable to not waste memory if 64 byte CAN messages are not used (on STM32 the elements are fixed size for up to 64 bytes CAN data), we can use the modm:architecture:can:message.buffer option to determine the necessary buffer element sizes.

Thus I suggest to adapt this driver, get rid of the software-buffers in favor of the large hardware-buffers and significantly less memory copying.

image

@rleh rleh marked this pull request as ready for review February 23, 2023 04:04
@rleh rleh added ci:hal Triggers the exhaustive HAL compile CI jobs and removed ci:hal Triggers the exhaustive HAL compile CI jobs labels Feb 23, 2023
salkinium
salkinium requested changes Apr 7, 2023
View reviewed changes
examples/samv71_xplained_ultra/can_dual/main.cpp Outdated Show resolved Hide resolved
examples/samv71_xplained_ultra/can_dual/main.cpp Outdated Show resolved Hide resolved
src/modm/platform/can/atsam-fdcan/can_impl.hpp Outdated Show resolved Hide resolved
src/modm/platform/can/atsam-fdcan/message_ram.hpp Outdated Show resolved Hide resolved
src/modm/platform/can/atsam-fdcan/message_ram.hpp Outdated Show resolved Hide resolved
src/modm/platform/can/atsam-fdcan/module.lb Outdated Show resolved Hide resolved
src/modm/architecture/module.lb Outdated Show resolved Hide resolved
@rleh rleh modified the milestones: 2023q1, 2023q2 Apr 8, 2023
@rleh rleh force-pushed the feature/fdcan_atsam branch 2 times, most recently from 173523d to f43ca14 Compare April 20, 2023 23:19
@rleh
Copy link
Member Author

rleh commented Apr 20, 2023 •
edited

The pull request is now ready for reviewing and merging :)

@chris-durand
Copy link
Member

I'll review tomorrow.

chris-durand
chris-durand reviewed Apr 21, 2023
View reviewed changes
Copy link
Member

@chris-durand chris-durand left a comment

Choose a reason for hiding this comment

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

Very nice!

src/modm/platform/can/sam-mcan/can.hpp Outdated Show resolved Hide resolved
src/modm/platform/can/sam-mcan/can.hpp Outdated Show resolved Hide resolved
src/modm/platform/can/sam-mcan/can.hpp Outdated Show resolved Hide resolved
src/modm/platform/can/sam-mcan/can.hpp Outdated Show resolved Hide resolved
src/modm/platform/can/sam-mcan/can_impl.hpp Outdated Show resolved Hide resolved
src/modm/platform/can/sam-mcan/can_impl.hpp Outdated Show resolved Hide resolved
src/modm/platform/can/sam-mcan/can_impl.hpp Outdated Show resolved Hide resolved
src/modm/platform/can/sam-mcan/can_interrupt.cpp.in Outdated Show resolved Hide resolved
src/modm/platform/can/sam-mcan/message_ram.hpp Outdated Show resolved Hide resolved
src/modm/platform/can/sam-mcan/message_ram.hpp Outdated Show resolved Hide resolved
salkinium
salkinium reviewed Apr 22, 2023
View reviewed changes
Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

Very well written, just minor nitpicks.

src/modm/platform/can/sam-mcan/can.hpp Outdated Show resolved Hide resolved
src/modm/platform/can/sam-mcan/can.hpp Outdated Show resolved Hide resolved
src/modm/platform/can/sam-mcan/can_impl.hpp Outdated Show resolved Hide resolved
src/modm/platform/can/sam-mcan/can_impl.hpp Outdated Show resolved Hide resolved
src/modm/platform/can/sam-mcan/can_impl.hpp Show resolved Hide resolved
src/modm/platform/can/sam-mcan/can_impl.hpp Show resolved Hide resolved
src/modm/platform/can/sam-mcan/can_impl.hpp Show resolved Hide resolved
src/modm/platform/can/sam-mcan/can_impl.hpp Outdated Show resolved Hide resolved
src/modm/platform/can/sam-mcan/can_impl.hpp Outdated Show resolved Hide resolved
src/modm/platform/can/sam-mcan/can_impl.hpp Outdated Show resolved Hide resolved
chris-durand and others added 3 commits May 8, 2023 13:38
@chris-durand @rleh
[architecture] Fix compiler warnings in can message, more constexpr
853d5c7 
Fix deprecated warning about implicitly-declared copy constructor with
custom assignment operator.
@rleh
[architecture] can: validate message buffer length and fix module dep…
ad6103f 
…endencies
@chris-durand @rleh
[sam] Add PCK configuration to PMC driver and fix enableUPll()
590d2b6 
Co-authored-by: Raphael Lehmann <raphael@rleh.de>
@rleh
Copy link
Member Author

rleh commented May 8, 2023

Merge now? 😆

@chris-durand
Copy link
Member

Merge now? 😆

I'd like more CAN filters 😅, but apart from that, feel free to merge.

@salkinium
Copy link
Member

(just cherry-picked some commits to fix the macOS CI)

@salkinium salkinium added the ci:hal Triggers the exhaustive HAL compile CI jobs label May 8, 2023
@rleh rleh removed the ci:hal Triggers the exhaustive HAL compile CI jobs label May 8, 2023
chris-durand
chris-durand approved these changes May 8, 2023
View reviewed changes
Copy link
Member

@chris-durand chris-durand left a comment

Choose a reason for hiding this comment

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

Thanks!

@rleh rleh merged commit bfafcd3 into modm-io:develop May 9, 2023
@rleh rleh deleted the feature/fdcan_atsam branch May 9, 2023 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@salkinium salkinium salkinium approved these changes

@chris-durand chris-durand chris-durand approved these changes

Assignees
No one assigned
Labels
feature 🚧
Milestone
2023q2
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rleh @chris-durand @salkinium