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

Add BSP for Adafruit Feather M4 CAN Express #658

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jboynes
Copy link
Contributor

@jboynes jboynes commented Jan 5, 2023

Summary

Adds a BSP for the Adafruit Feather M4 CAN Express board.

Blinky, Neopixel and USB examples have been tested on actual hardware.

There was some discussion in #622 about using features for CAN support rather than adding a new BSP. However, as this board uses a SAME51 part (rather than the SAMD on the M4 Express) I thought it made sense to keep it separate as it would have dependencies on a different PAC.

The CAN module is not yet exposed in the BSP. My intention would be to support the embedded_can HAL using the mcan crate (cf. #654).

Checklist

  • CHANGELOG.md for the BSP or HAL updated
  • All new or modified code is well documented, especially public items
  • No new warnings or clippy suggestions have been introduced (see CI or check locally)

If Adding a new Board

  • Board CI added to crates.json
  • Board is properly following "Tier 2" conventions, unless otherwise decided to be "Tier 1"

@bradleyharden
Copy link
Contributor

bradleyharden commented Jan 5, 2023

@jboynes, using a different chip/BSP is definitely a stronger reason to have a different BSP. But it's also not a total dealbreaker to reuse the existing BSP. Other than CAN, are there any other major differences between the E51J and D51J chips? And what about the boards themselves? Are the pinouts and peripherals essentially identical between the two boards? Or is there variation there as well?

We might be able to get away with some hacking in Cargo.toml, which I think would benefit everyone. You would get free Tier 1 support and there would be less code duplication.

@jboynes
Copy link
Contributor Author

jboynes commented Jan 5, 2023

To my knowledge, the only difference between the D51J and E51J is the presence of the CAN module.

The chip identifies differently which could affect debugging (e.g. the "chip" value in [metadata]). By default though, flashing is done using hf2 which does not rely this. The board does have pads to attach a SWD probe though.

Differences on the board compared to the M4 are:

  • battery pin V_DIV changed from PB01 to PB00
  • neopixel data pin D8_NEOPIX changed from PB03 to PB02
  • PB03 is now used as power for the neopixel NEO_PWR

The on-board CAN transceiver uses the following pins which are unused on the basic M4:

  • PB13 is used to enable the +5V supply for the CAN transceiver BOOST_EN
  • PB12 is used to put the CAN transceiver in standby mode CAN1_S
  • PB14 and PB15 are used for CAN data CAN1_TX and CAN1_RX

Another advantage of having this as a separate BSP crate is that it can version separately from the feather_m4 one, especially as the embedded_can HAL is still relatively new so there are likely to be CAN-specific changes that won't affect most users. It would also allow the base BSP to be updated with needing to test on CAN hardware.

@glaeqen
Copy link
Contributor

glaeqen commented Jan 5, 2023

I'm quite curious how this BSP will develop. My understanding is that you want to implement embedded-can on mcan in some opinionated way that will kind of work for toy projects. As far as my knowledge goes embedded-can is quite limited and makes a lot of assumptions (like that TX and RX is supposed to use the same frame type which in MCAN is not that straightforward). This is why we only use primitives from embedded-can, like Id enums and such.

@jboynes
Copy link
Contributor Author

jboynes commented Jan 6, 2023

I'm quite curious how this BSP will develop. My understanding is that you want to implement embedded-can on mcan in some opinionated way that will kind of work for toy projects. As far as my knowledge goes embedded-can is quite limited and makes a lot of assumptions (like that TX and RX is supposed to use the same frame type which in MCAN is not that straightforward). This is why we only use primitives from embedded-can, like Id enums and such.

I don't have many opinions here 🤷. My thought was that embedded_hal was using embedded_can so that would be a natural way to go. I agree the API looks fairly basic at the moment (e.g. I see no support for CAN-FD, filters, priorities, etc.) which just means there's work to do. That's mainly why I thought that any BSP for a board with CAN would see more churn than one without. It's also a case for not making this a Tier 1 BSP, at least for now.

Having more boards supported would allow more testing of the HAL and its abstractions. This PR adds a SAM E51-based board, and I've proposed adding a SAM C21 (Cortex-M0) board as well for some diversity in Microchip/Atmel devices. And of course, there are other devices using Bosch's M_CAN IP as well (e.g. the STM32G4 (cf. fdcan) and LPC5516 MCUs, or the TCAN4550 external controller).

@bradleyharden
Copy link
Contributor

@jboynes, I think this case is right on the deciding line. The small board differences could be handled with #[cfg] attributes in the bsp_pins! macro, and selecting the correct chip variant could be done with a Cargo feature. But I don't see any way to change the package metadata. That's probably the biggest driver to using a separate BSP. However, by going the separate route, you give up Tier 1 support and increase code duplication. We could eventually consider upgrading the board to Tier 1, but it probably wouldn't happen immediately.

I think I'll leave it up to you (and possibly anyone else who wants to weigh in). Would you prefer this as a separate BSP? If so, we can merge it.

@jboynes
Copy link
Contributor Author

jboynes commented Jan 6, 2023

Would you prefer this as a separate BSP? If so, we can merge it.

I think it would be preferable for now, because:

  • It can be Tier 2
  • Being Tier 2, it can iterate more quickly in response to changes in the CAN HAL
  • It can still be upgraded to Tier 1 when things stabilize

There's not a lot of production code to maintain in the BSP itself. I think most of the maintenance work would be in tracking dependencies. Ideally, the BSP would only define devices on the board and the pin mappings that connect to them; things that are board specific, and only things that are board specific.

I will explore if there's a way to refactor this to reduce the duplication. I'll take a look at unifying with the atsme54_xpro as that's also Tier 2 and would also be affected by the CAN support.

@jbeaurivage jbeaurivage added the board support Related to support for a particular board label May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
board support Related to support for a particular board
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants