ports/mimxrt: Add machine.CAN driver. #12324
Conversation
0182d4b to
eaf5776
Compare
|
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. |
We wanted to first see if there are any comments on the API, since it's a new |
|
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. |
|
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? |
|
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 |
|
At a short glance, the API here is at the level of the internal-API of |
|
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. |
|
Could this PR be the start of a common |
|
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. |
Signed-off-by: Kwabena W. Agyeman <kwagyeman@live.com>
eaf5776 to
1ba6ef5
Compare
|
Docs added. |
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.
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 |
|
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). |
|
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:
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 |
|
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 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 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. |
You missed one more option, we can just move this code to
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 |
|
Regarding a common |
I did, thanks @projectgus that sounds great! |
|
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. |
| { MP_ROM_QSTR(MP_QSTR_SILENT), MP_ROM_INT(CAN_SILENT_FLAG) }, | ||
| { MP_ROM_QSTR(MP_QSTR_SILENT_LOOPBACK), MP_ROM_INT(CAN_LOOPBACK_FLAG | CAN_SILENT_FLAG) }, | ||
| { MP_ROM_QSTR(MP_QSTR_DUAL), MP_ROM_INT(CAN_DUAL) }, | ||
| { MP_ROM_QSTR(MP_QSTR_MASK32), MP_ROM_INT(CAN_DUAL) }, |
|
This is an automated heads-up that we've just merged a Pull Request See #13763 A search suggests this PR might apply the STATIC macro to some C code. If it Although this is an automated message, feel free to @-reply to me directly if |
|
hello @kwagyeman I pulled your PR and tested it on mimxrt1060_evk, the loopback mode doesn't work.
|
|
@tonyzhangnxp - Thanks for the note. I can debug that whenever MicroPython finishes a CAN spec. |
|
I built micropython for ternsy 4.1 on the branch with this PR, but haven't got very far with usage, simple things like: Other things in the repl seem to work as expected. Are there any tips to get more verbose output? I can see this interface wraps the nxp driver and exposes can bus |
|
@kwagyeman can you share some examples of your can usage? I built this branch, but the driver hung when opening fifo 1 on a teensy 4.1 when using some of the simple examples from the micropyrhon can docs. I suspect it was the loop forever waiting for an idle bus in the nxp driver, but a working example of usage would be helpful before i dust off a jlink and dig into debugging with gdb 🙂 I tried using invalid args, expecting to see some errors reported but as far a i recall and symptoms were the constructor hanging forever until the board was reset. Could be a build issue, but other micropython features work as expected. |
|
Thanks, that helped confirm I had a build issue. I've now also rebased your commit near master and it's looking promising in initial testing. |
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.