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

H7 FDCAN driver. #4793

Open
wants to merge 3 commits into
base: master
from

Conversation

@iabdalkader
Copy link
Contributor

commented May 17, 2019

  • This is submitted for review and edits related to issue #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...

@dpgeorge

This comment has been minimized.

Copy link
Member

commented May 22, 2019

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

@iabdalkader

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

@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

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Ok, I'll look into it.

@dpgeorge

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

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

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

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

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

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

@iabdalkader iabdalkader force-pushed the iabdalkader:h7_fdcan branch from a59f7df to b2b7275 Sep 12, 2019

@iabdalkader

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

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 force-pushed the iabdalkader:h7_fdcan branch from b2b7275 to 22099ab Sep 14, 2019

@iabdalkader iabdalkader reopened this Sep 14, 2019

@iabdalkader

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2019

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

@iabdalkader

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

@dpgeorge It's done.

@dpgeorge

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

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

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.