Swap to bxCan crate #207
Swap to bxCan crate #207
Conversation
|
Also note: this is definitely a breaking change. I don't think thats a huge deal given that can was behind a feature flag anyhow and was fairly experimental. |
No worries. Breaking changes for now are just important to note in the changelog. Bumping the version number is not a big deal. :) |
|
Could you please change the version requirement to ">=0.4" or ">=0.4, <0.6" for reasons described at stm32-rs/stm32f7xx-hal#114 (comment). Discussion also available at stm32-rs/stm32f4xx-hal#285. |
zklapow
force-pushed
the
zklapow-bxcan
branch
2 times, most recently
from
a062975 to
64856ee
Compare
|
Apologies (as usual) it took me a bit to get back to this. Should be up to date with master now with the bxcan version issue fixed, and ready for another look (or merging). |
| filters.enable_bank(0, Mask32::accept_all()); | ||
|
|
||
| // Enable filters. | ||
| drop(filters); |
There was a problem hiding this comment.
This seems confusing. Has the Drop trait the site effect of applying the filter?
There was a problem hiding this comment.
Ah I see, filter holds a mutual reference to can, so can can not be used later, as long as the reference is held. Is this correct? And does it still compile without this drop?
| /// Release owned peripherals | ||
| pub fn free( | ||
| self, | ||
| ) -> ( | ||
| pac::CAN, | ||
| gpioa::PA11<AF9<PushPull>>, | ||
| gpioa::PA12<AF9<PushPull>>, | ||
| ) { | ||
| (self.can, self._rx, self._tx) | ||
| } | ||
| } |
There was a problem hiding this comment.
Hm. I see, the bxcan crate does consume this peripheral and is not able to free it. But I would still keep this function and submit a PR to the bxcan crate, that it also supports freeing the peripheral again.
There was a problem hiding this comment.
bxcan 0.5.1 now has a Can::free method, so we can use it here. So bxcan instance would return fn free(bxcan::Can<Self>) -> Self, where Self itself implements pub fn free(self) -> (pac::CAN, gpioa::PA11<...>, gpioa::PA12 <...>).
This would mean, we have to call free twice to get back the gpios: let (can, pa11, pa12) = can.free().free(); but I think, it is not too bad.
src/can.rs
Outdated
| pub struct Can<TX, RX> { | ||
| _can: pac::CAN, | ||
| _tx: TX, | ||
| _rx: RX, |
There was a problem hiding this comment.
When the free() method is implemented, the underscore should'nt be necessary anymore:
| pub struct Can<TX, RX> { | |
| _can: pac::CAN, | |
| _tx: TX, | |
| _rx: RX, | |
| pub struct Can<TX, RX> { | |
| can: pac::CAN, | |
| tx: TX, | |
| rx: RX, |
| // APB1 (PCLK1): 64MHz, Bit rate: 500kBit/s, Sample Point 87.5% | ||
| // Value was calculated with http://www.bittiming.can-wiki.info/ | ||
| can.modify_config() | ||
| .set_bit_timing(0x001c_0003) |
There was a problem hiding this comment.
Minor nit: Could you give this magic value a name? I see, that your comment explains the reason for this value, but I (as someone who has not really looked into can) do not fully grasp the meaning of each single bit.
examples/can.rs
Outdated
| @@ -13,7 +13,9 @@ use hal::pac; | |||
| use hal::prelude::*; | |||
| use hal::watchdog::IndependentWatchDog; | |||
|
|
|||
| use hal::can::{Can, CanFilter, CanFrame, CanId, Filter, Frame, Receiver, Transmitter}; | |||
| use bxcan::filter::Mask32; | |||
| use bxcan::{Frame, StandardId}; | |||
There was a problem hiding this comment.
Could be
| use bxcan::{Frame, StandardId}; | |
| use can::bxcan::{Frame, StandardId}; |
if we would reexport the can module. (See other comment)
|
How about returning |
Co-authored-by: Fabian <[email protected]>
|
I've added a free method and changed some minor nits and rebased it onto current master. Thanks for your contribution! :) |
I said I wasn't gonna have time for this, but it turns out this was a lot easier than I realized.
Currently (as before) we only support pin defs for the 302/303, but it should be trivial to add more when thats desired.