8000 H7 FDCAN driver. by iabdalkader · Pull Request #4793 · micropython/micropython · GitHub
[go: up one dir, main page]
Skip to content

H7 FDCAN driver. #4793

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
Closed
wants to merge 3 commits into from
Closed

H7 FDCAN driver. #4793

wants to merge 3 commits into from

Conversation

iabdalkader
Copy link
Contributor
@iabdalkader iabdalkader commented May 17, 2019 •
edited
Loading

  • This is submitted for review and edits related to issue STM32H7 CAN FD partial and full support #4774

  • This driver is implemented in fdcan.c but it defines pyb_can_type, so from a user's perspective it's same as can.c (ex. from pyb import CAN works the same). fdcan.c is not compiled unless HW_FDCAN_TX/RX is defined and it shares can.h with can.c, so there's no need for an additional fdcan_init0()/deinit etc...

  • A new GPIO function (FDCAN) had to be added, otherwise the AF_FN_CAN couldn't be found in runtime, or the build would fail if AF_FN_FDCAN is used.

  • The "Message RAM" for FDCAN 1 and 2 is shared and a memory offset can be configured for CAN controller memory. I split the memory in half between the two CANs by using memory_size/2 as offset for CAN2. This offset simplifies managing things like filter indices (which always start from 0->64 for either CAN controller), see RM0433 56.3.2

  • The setfilter function only supports the RANGE mode. This allows a range of IDs, ex. can.setfilter(index, fifo, (id_start, id_end). Filters support other modes like dual IDs, bit masking ID and other configuration modes, see RM0433 56.3.19.

  • This driver has been tested in loopback mode and with a two node transceiver-less network. However, it could use a lot of improvements especially with filters, interrupts and error handling, rxcallback etc...

8000
@dpgeorge
Copy link
Member

Thanks very much for this. After #4784 I will look at this in detail.

@iabdalkader
Copy link
Contributor Author
iabdalkader commented Aug 31, 2019 •
edited
Loading

I'm going to continue working on this now, and I have CAN transceivers to test with. I will rebase to fix conflicts, but I need to know if adding a new driver like this is okay or do you prefer to support them both in the same driver ? @dpgeorge

@iabdalkader
Copy link
Contributor Author

Update: I tested this driver with transceivers and it's working fine, also fixed a bug with any() and filter acceptance and added support for the other filter types.

@iabdalkader
Copy link
Contributor Author
iabdalkader commented Sep 9, 2019 •
edited
Loading

@dpgeorge I really could use some help/hints here, as I want to merge this in our next release (which is overdue) and I don't want to diverge too much from upstream.

@dpgeorge
Copy link
Member

Ok, I'll look into it.

@dpgeorge dpgeorge mentioned this pull request Sep 11, 2019
Closed
@dpgeorge
Copy link
Member

but I need to know if adding a new driver like this is okay or do you prefer to support them both in the same driver ?

I can understand why you added fdcan.c, because it's a bit different to the existing can.c. But I do thing there are enough similarities (especially with the Python-level API) that these files can be merged and can.c can support both CAN and FDCAN peripherals (similar to how sdcard.c also supports MMC).

I simplified this PR in #5096, trying to reduce the diff. There are less files changed (AF and pin stuff remain the same, with a small #define to make it work), and the diff between can.c and fdcan.c is smaller by about 10% (minor changes, nothing functional).

An alternative to merging the code together in can.c would be to have 3 files:

  • can.c with low-level helper functions for CAN periph
  • fdcan.c with low-level helper functions for FDCAN periph
  • pyb_can.c with Python-level wrapper functions, using low-level helpers from above files

Doing that probably requires a similar effort to merging can.c with fdcan.c. And it's probably a bit cleaner to do it this way.

@iabdalkader
Copy link
Contributor Author

I simplified this PR in #5096, trying to reduce the diff. There are less files changed (AF and pin stuff remain the same, with a small #define to make it work),

This is much better, but I think the test should be for MICROPY_HW_ENABLE_FDCAN because other (future) micros may have FDCAN as well. Maybe define in mpconfigboard_common.h for STM32H7 ?

An alternative to merging the code together in can.c would be to have 3 files:

  • can.c with low-level helper functions for CAN periph
  • fdcan.c with low-level helper functions for FDCAN periph
  • pyb_can.c with Python-level wrapper functions, using low-level helpers from above files

I like this approach more, but shouldn't it be called machine_can.c now instead ?

@dpgeorge
Copy link
Member

but I think the test should be for MICROPY_HW_ENABLE_FDCAN because other (future) micros may have FDCAN as well. Maybe define in mpconfigboard_common.h for STM32H7 ?

That sounds good.

I like this approach more, but shouldn't it be called machine_can.c now instead ?

Ok, let's go for this approach then, since it's more sustainable for the future, and matches how other peripherals are evolving.

But it should be pyb_can.c because this class is very specific to the stm32 port. One day we can define machine.CAN, which would also work on the esp32 and other targets, and then utilize stm32/can.c and stm32/fdcan.c to implement it on stm32.

@dpgeorge dpgeorge mentioned this pull request Sep 12, 2019
Closed
@dpgeorge
Copy link
Member

See #5100 for an initial attempt at splitting out Python bindings to pyb_can.c (no FDCAN support though).

@iabdalkader
Copy link
Contributor Author

See #5100 for an initial attempt at splitting out Python bindings to pyb_can.c (no FDCAN support though).

Can we move more low-level code into can.c and fdcan.c ? especially where they differ most, like init and setfilter code. For example, init would look like this:

    if (!can_init(self, args[ARG_mode].u_int, args[ARG_prescaler].u_int,
                args[ARG_sjw].u_int, args[ARG_bs1].u_int), args[ARG_bs2].u_int, args[ARG_auto_restart].u_bool) {

Otherwise it looks good to me, if you merge that I'll rebase and add fdcan.c

@iabdalkader iabdalkader reopened this Sep 14, 2019
@iabdalkader
Copy link
Contributor Author

@dpgeorge I pulled changes from your PRs into this branch, and I'm adding FDCAN support... Will push more commits later.

@iabdalkader
Copy link
Contributor Author

@dpgeorge It's done.

@dpgeorge
Copy link
Member

It's done.

Ok, thanks. I see that it could probably be factored a bit more, more low-level stuff in can.c/fdcan.c, but that doesn't have to be done now.

I'll review this fully as soon as I can.

@iabdalkader
Copy link
Contributor Author

I see that it could probably be factored a bit more, more low-level stuff in can.c/fdcan.c, but that doesn't have to be done now.

Yes I couldn't agree more, especially pyb_can_init_helper, pyb_can_setfilter and pyb_can_send, but I already changed too much code in this PR.

Note I tested sending, receiving and setfilter on F7 and H7, it works fine but one minor issue with setfilter it uses different constants for CAN and FDCAN. I'm not sure what would be the best way to fix this. Anyway, I'm going to merge this into our branch please let me know if it needs more work.

@dpgeorge
Copy link
Member

I had more of a look at this PR and it looks fine for an initial working version.

setfilter it uses different constants for CAN and FDCAN

Yeah I looked at the datasheet and indeed the FDCAN hardware is different with the way it does filtering:

  • RANGE mode: could be mapped to MASK32 but not in a fully general way
  • DUAL mode: could be mapped to LIST32, just a list of 2 allowed id's
  • MASK mode: could be mapped to MASK32 but probably not in a general way as the masking behaviour is slightly different to the original CAN peripheral

Well, the aim of pyb-modules is to provide a low-level interface to the hardware, and it's not possible to do that if the underlying hardware differs among MCUs. Since we don't want to restrict too much how the underlying FDCAN is used I think it's fair enough to allow the config of the filters to differ here. At least the signature of setfilter() will be the same for both versions, it's just the constants (LIST16, MASK16 vs RANGE, DUAL, MASK) will differ.

In summary I'm happy to have different constants and setfilter behaviour.

@iabdalkader
Copy link
Contributor Author

I think it's fair enough to allow the config of the filters to differ here. At least the signature of setfilter() will be the same for both versions, it's just the constants (LIST16, MASK16 vs RANGE, DUAL, MASK) will differ.

In summary I'm happy to have different constants and setfilter behaviour.

Then it should be good to merge now I think, otherwise let me know if there's something else that needs to be done in this PR.

@dpgeorge
Copy link
Member

Ok, this driver was merged in f7a07b3, support for FDCAN on NUCLEO_H743ZI merged in 4f78ba3

@dpgeorge dpgeorge closed this Sep 23, 2019
@dpgeorge
Copy link
Member

@iabdalkader thanks for the effort here. Feel free to follow up with any fixes as you find them.

@iabdalkader iabdalkader deleted the h7_fdcan branch September 24, 2019 12:33
tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Jul 21, 2021
@dhalbert
Merge pull request micropython#4793 from kmatch98/funhouse_spi_speed
5e773b8 
reduce SPI display baudrate from 60 MHz to 5 MHz to eliminate display glitch
@iabdalkader iabdalkader mentioned this pull request Aug 30, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
port-stm32
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@iabdalkader @dpgeorge
0