ports/mimxrt: Add machine.CAN driver.

[kwagyeman]

This is a machine.CAN driver for the IMXRT that mimicks the PYB.CAN module on the STM32.

I didn't have an API to work against so I made the driver just re-implement as much as possible from the STM32 PYB.CAN driver. FDCAN is absent though since I did not have a chip that supported FDCAN to test with. Regular CAN does work however in all ways, shapes, and forms.

[kwagyeman]

@iabdalkader

[robert-hh]

Thank you for taking the time to implement this module. A few people told to do so, but did not find the time for it. I just made y quick check by compiling it for all MCU types supported by the MIMXRT port, including the 1010 and 1015 boards, even if they do not support CAN. I have no CAN hardware for further tests. You could add a short extension to the MIMXRT documentation for the CAN class and it's methods.

[iabdalkader]

You could add a short extension to the MIMXRT documentation for the CAN class and it's methods.

We wanted to first see if there are any comments on the API, since it's a new machine API, from you and @dpgeorge , if all is good we'll go ahead and document it and then merge it in our fork.

[robert-hh]

I would see writing a documentation as a quality assurance step. Writing a documentation causes a re-consideration of the design decision. And with a documentation the intended API is more clear.
b.t.w.: Looking through the methods there is CAN.rxcallback() as method. Would that not usually be a keyword option of the init() method and the constuctor?

[andrewleech]

Not meaning to throw a spanner in the works, but I just want to raise a question about the appropriate CAN api to use here.

My understanding of the stm one is that it's getting just a thin wrapper on the stm hal / can peripheral interface.

tangent; I've been wanting to work on getting canopen going on micropython. The obvious way to tackle this would be to port the open source big-python canopen library: https://github.com/christiansandberg/canopen

It's layered on top of a different driver library for desktop computer can interfaces: https://github.com/hardbyte/python-can

It would be interesting to compare the API of python-can to the stm can here and see if the python library interface makes more sense to target?

[iabdalkader]

Honestly other than minor changes to the API and the docs, we really can't allocate any more time for this task, so if it's going to take some time to decide on a machine.CAN API, or if you already have something in mind that you want to implement, we can just move this code out of the way to mimxrt_can.c and modmimxrt.c (similar to pyb_can.c and modpyb.c), we still need a working CAN driver for mimxrt anyway which this PR provides.

[robert-hh]

At a short glance, the API here is at the level of the internal-API of python-can. That could allow implementing/porting python-can as Python module. The MIMXRT devices usually have sufficient RAM and CPU speed to support it.

[andrewleech]

Thanks @robert-hh I haven't had a chance to look myself and aren't very familiar with CAN protocol to know exactly where to start.

It's worth noting that python-can is gpl (while canopen is permissive) so I am hoping to avoid using it directly, but that certainly doesn't rule out writing a basic api compatible variant of it for micropython-lib that adapts from pyb.can or compatible to the higher level desktop compatible libraries.

[IhorNehrutsa]

Could this PR be the start of a common machine.CAN API?
docs\machine_CAN: Add CAN docs. #12330

[kwagyeman]

Hi all,

This module is literally a mirror of the PYB CAN module on the STM32. So, I implemented all methods mimicking the arguments and etc. of PYB CAN.

Personally, I think the PYB.CAN module has... a ugly API. However, it's what's been deployed for years and many customers have written code for it. It would be very customer unfriendly to change the API.

I think CANOpen would be great. But, that should probably be implemented ontop of this module as a micropython library and whatever features that aren't offered by this module should be added to support a CANOpen MicroPython library.

Regarding what Ibrahim mentioned. This work was done to support the launch of the OpenMV Cam RT1062. We have a lot of customers who use CAN with the STM32 line of OpenMV Cams. I don't have time to work on the CAN module anymore. So, if the API needs to change we will maintain it privately.

...

It looks like this author: https://github.com/micropython/micropython/pull/12330/files did more or less the same thing as me. They made their API copy from PYB.CAN. It also appears they added some extra methods.

...

Anyway, I can add the documentation to this PR. Will do that now.

[kwagyeman]

Docs added.

[andrewleech]

I think CANOpen would be great. But, that should probably be implemented ontop of this module as a micropython library and whatever features that aren't offered by this module should be added to support a CANOpen MicroPython library.

I totally agree that CANOpen can and should be a separate python module running on top of the underlying CAN peripheral interface! Ideally the cpython canopen library could eventually be used unmodified with an appropriate shim/wrapper library to adapt the lower level interfaces.

Personally, I think the PYB.CAN module has... a ugly API. However, it's what's been deployed for years and many customers have written code for it. It would be very customer unfriendly to change the API.

Yep I also though the pyb.CAN interface was strange and hard to understand, even just the couple of times I've helped colleagues get going with it I had a hard time getting started myself. The one and only other "lower level" can api I know to compare against is python-can hence my suggestion - but yeah comparing the two interfaces and understanding the differences has been on my backlog for a long time now.... I'm not finding the time to even look at them!

So yeah as much as I'd prefer a machine.CAN interface to be "nicer" for people to use, I don't know what that should look like really. Matching the pyb.CAN interface is still better than presenting another completely randomly new API so good work there.

[kwagyeman]

Oh, something to note about this CAN driver that is an improvement over the STM32 one.

So, it has a FIFO interface that's 6 CAN messages deep for RX. However, on TX I use all available message buffers to send CAN messages (like 30 of them on the RT1062). When sending a message I re-use the same buffer if you are sending the same ID to keep data in-order. This means send() will not generate out-of-order transactions as long as the ID doesn't change that's being sent.

The STM32 driver doesn't do this. I think I created a bug ticket about this a couple of years back. Not sure if it was fixed.

Anyway, you should notice the driver is rather high performance and uses the CAN system to it's maximum (per what the API allows).

[dpgeorge]

Thanks @kwagyeman for the effort to implement this and for raising a PR. I do think it's a good idea for OpenMV's fork to be more closely aligned with upstream MicroPython (ie have less differences), so in general making PRs for these additions that you need to support your hardware is welcome.

That said, it's not so helpful to essentially force our (us maintainers) hand by painting us into a corner where we must make hard decisions. You say that "I don't have time to work on the CAN module anymore. So, if the API needs to change we will maintain it privately." And essentially this means we have to decide to either:

• merge this code with only minor changes, and accept a poor API that probably won't fit well with other MCUs/ports
• not merge this code, take a bit of time to come up with a cleaner API, and then be incompatible with OpenMV's fork

Both of these options have long term consequences which are unpleasant for everyone who wants to use CAN. And requiring us to act so quickly sets a bad precedent for others who contribute to MicroPython.

I understand that you have hardware to ship and people want to use CAN, and in the short term the easiest thing for OpenMV is to just take the code here and run with it. I also understand that contributing big changes/additions to MicroPython takes a lot of effort and time (from both contributors and maintainers) and things can move rather slowly around here. But that's because developing for embedded systems is hard, and we always err on the side of doing things the "proper" way (even if it doesn't always work out perfect).

We also have a lot of other things in the queue, and it's not reasonable to postpone everything else to work on CAN.

Don't get me wrong though, we would love to spend effort on CAN because it is important and useful for embedded, and we have had lots of requests to improve MicroPython's support for it. We just need to find time for it and try and do it justice, in terms of making a nice API. Doing that is probably not going to work well for OpenMV in the short term (eg if you decide to wait for a redesigned API) but doing it the "proper" way and making a nice machine.CAN class will be a good long term outcome for everyone.

[kwagyeman]

Hi @dpgeorge,

I understand your position. However, as mentioned, I do have to ship CAN support for OpenMV Cam RT1062. We are happy to use whatever new machine.CAN module you come up with when that work is finally complete. I know that you created the machine module in the first place because pyb was too hardware specific to the pyboard.

As regards to this PR, I guess the best thing for us to do would be to rename the module PYBCAN or something like that under the machine module in the mean-time and then deprecate it once you have a replacement CAN module. While it's not the friendliest to users... it won't really be a huge issue as folks are much more willing to update their code than deal with a missing feature. Typically, when folks deploy their cameras they don't really get firmware updates anymore anyway. I just wrote this to mimick the PYB CAN API since that's what existed already and would give users the least work to deal with.

However, we do have about a month or so until hardware comes out. If you can suggest some simple straight forward changes to the API that I can make I'm happy to do them now. See the docs for what the API is: https://github.com/micropython/micropython/pull/12324/files#diff-5ff2224e1bcd64d0740aa1c09f9b619c76797b8d095202bc31df9df4d7b6224e

E.g. I'd love to remove the fifo argument to many of the functions as I check if it's not zero and then error out on that. It has no use for the IMXRT10XX.

Anyway, as a long-time MicroPython sponsor, could we use the few hours you promised per month for our tier to suggest a clean API soon? Assuming the changes aren't huge it shouldn't be a problem for me to cleanup the API.

[iabdalkader]

And essentially this means we have to decide to either:

* merge this code with only minor changes, and accept a poor API that probably won't fit well with other MCUs/ports

* not merge this code, take a bit of time to come up with a cleaner API, and then be incompatible with OpenMV's fork

You missed one more option, we can just move this code to mimxrt_can.c and modmimxrt.c similar to stm32's pyb_can.c and modstm.c, and when a consensus is reached on a machine.CAN API eventually, this code can be reused. This way we don't have to rush into an API and when/if someone is willing and has the time to do a proper machine.CAN API they can do so.

That said, it's not so helpful to essentially force our (us maintainers) hand by painting us into a corner where we must make hard decisions

I don't think it's fair for us either to be forced into taking on a bigger task than what we initially were willing to contribute and what our time allowed us to do, at the moment we just don't have the capacity to get into an extended API/RFC type of discussion that could very well drag on for a year or so, in the past I did spend a considerable amount of time adding support for FDCAN, improving CAN driver and its documentation for the stm32 port (in #8185, #4793 and #6212) we just can't do this right now knowing how much effort it could take.

[projectgus]

Regarding a common machine.CAN API, we've laid out some reasoning and a plan at #12337. Please take a look. Will definitely refer to the implementation offered here and the specific requirements for the MIMXRT CAN peripheral when designing it.

[iabdalkader]

Regarding a common machine.CAN API, we've laid out some reasoning and a plan at #12337. Please take a look.

I did, thanks @projectgus that sounds great!

[kwagyeman]

Awesome! Works for me. I'm pretty confident in how to use the CAN driver now so it shouldn't be too hard to adapt it.

[kwagyeman]

Should be LIST32.

[projectgus]

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

[tonyzhangnxp]

hello @kwagyeman

I pulled your PR and tested it on mimxrt1060_evk, the loopback mode doesn't work.
Normal mode works well.
`

can = CAN(0,CAN.LOOPBACK)
can.setfilter(0, CAN.LIST32, 0, (123, 124))
can.send('message!', 123)
can.recv(0)
Traceback (most recent call last):
File "", line 1, in
OSError: [Errno 116] ETIMEDOUT
`

[kwagyeman]

@tonyzhangnxp - Thanks for the note. I can debug that whenever MicroPython finishes a CAN spec.