CAN Support #100

zklapow · 2020-05-09T18:56:26Z

This PR is fairly incomplete still, though it is functional.

I created a (temp) create called embedded-hal-can to try implementing the traits currently in rust-embedded/embedded-hal#77 though that PR is so old now, I am not sure its going to get much traction (and I am not totally sure I love the structure of those traits anyhow) so I may remove that piece.

I have been testing this on an STM32f302C8T6, I am working on simplifying my "blinky with can" into an example of how to use this.

todos:

Generify this to support other stm32f3xx devices

future improvements:

Some sort of baud rate param heuristics (maybe just embed a table for standard rates?)
Improved filter handling (the current implementation "wastes" filter registers)

Wanted to post this now though as I have been sitting on it for a while and I figured it would be good to at least get thoughts/feedback.

Sh3Rm4n

Thank you for this PR!

I left some comments, but these are mostly nit-picks. I know this PR is still WIP, so ignore comments which are not applicable.

Disclaimer: I did not have much experience with Can nor did I look into the CAN hal proposal.

src/can.rs

Cargo.toml

src/can.rs

Sh3Rm4n · 2020-05-10T19:54:45Z

src/can.rs

+}
+
+impl CanFrame {
+    pub fn new_with_len(id: CanId, data: &[u8], length: usize) -> CanFrame {


Just curious. Why must the data slice be converted into an array? Slice has a len() method. Maybe you could give me some insight :)

I should likely limit the visibility of this method and it could be simplified a bit... The reason this exists is mainly to do with lifetimes and there are sort of two reasons for it:

is for reading, we allocate an array always of length 8 bytes (the max CAN frame size), because we cant do dynamic allocation and then use a subslice of it, but now that I think more about it, we can likely just consume that array in this case and avoid the reference.

Is because of how we need to implement embedded_hal_can::Frame. Having every frame have a 8 byte backing array was just the simplest way to handle this without getting into using generic_array or something, but ultimately I may do that, because it would make the code clearer.

zklapow · 2020-05-17T17:21:47Z

Added a simple "blinky" example, heres a GIF of it in action:

zklapow · 2020-05-30T20:12:08Z

@Sh3Rm4n this should be ready for another look when you have some time, it should be mergeable in its current state.

A couple notes:

I updated to use ArrayVec to give the CanFrame a slightly simpler API (no need to remember that there is an extra length field there!). I tried to use generic_array but I don't think that will be possible without having transmit be generic of the frame size. Given that a frame is at most 8 bytes, having to allocate the full backing array seemed OK. (I also considered introducing some sort of allocator dedicated to canframes but.... that seemed extreme)
Everything should be behind the can feature flag now, so it is disabled by default.

zklapow · 2020-07-12T22:28:06Z

@Sh3Rm4n any thoughts on another review or merging this?

teskje · 2020-07-15T19:04:18Z

I'm working on getting all the open PRs reviewed, starting with the oldest one. If @Sh3Rm4n is busy I'll take a look once I got to this one.

Just to confirm though: Do you consider this ready for merging? I'm asking because there are still two open TODOs.

zklapow · 2020-07-17T19:04:31Z

I moved those around a bit to be more clear, I do think they would be interesting to tackle in the future but are sort of hard and don't really affect the functionality here much. This should be useable and mergeable as-is.

Sh3Rm4n

Sorry for keeping you waiting. I've left some comments, but I did not dive into the implementation logic, yet.

Cargo.toml

Sh3Rm4n · 2020-07-19T21:08:07Z

examples/can.rs

+    let filter = CanFilter::from_mask(0b100, ID as u32);
+    rx0.set_filter(filter);
+
+    // Watchdog makes sure this gets restarted periodically if nothing happens


Nice, this is also now an example for the watchdog 👍

examples/can.rs

src/can.rs

Sh3Rm4n · 2020-07-19T21:18:15Z

src/can.rs

+
+static FILTER_INDEX: AtomicU8 = AtomicU8::new(0);
+
+pub struct Can {


I would like to see more comments / documentation, especially on the public interfaces, like this struct :)

src/can.rs

Co-authored-by: Fabian <[email protected]>

zklapow · 2020-08-06T23:46:23Z

I have updated this to address the open comments. I added a bunch of notes about the usages of unsafe but a lot of those notes are semi silly but calling bits() is just always unsafe but also the only way to access those registers.

David-OConnor · 2020-09-27T03:43:23Z

Merge!

zklapow · 2020-09-28T14:49:52Z

@David-OConnor would love to tho I dont have access 👍

@Sh3Rm4n or @ra-kete I have merged master to address some conflicts as well as updated the changelog, and the build is passing. Any chance one of you could merge this for me when you have a moment? I think all of the comments should be addressed.

teskje · 2020-09-28T15:06:24Z

@Sh3Rm4n has reviewed this, so if he gives his thumbs-up for merging I'm fine too.

Sh3Rm4n

Hey, sorry for the late response.

Minor nits and this is not addressed, yet.

But overall LGTM. :)

Sh3Rm4n · 2020-10-18T18:35:50Z

src/can.rs

+        return (transmitter, fifo0, fifo1);
+    }
+
+    pub fn release(self) -> stm32::CAN {


Suggested change

pub fn release(self) -> stm32::CAN {

pub fn free(self) -> stm32::CAN {

And it should release gpioa::PA11<AF9> and gpioa::PA12<AF9> as well.

zklapow · 2020-10-19T14:16:22Z

updated, the docs are still a bit sparse but unfortunately I don't have a ton of time to work on this ATM.

Sh3Rm4n · 2020-10-20T08:34:23Z

Nice. Thanks for the contribution. Finally i've time to merge this 🎉

Thanks for your patience. Further things can still be improved later on, if necessary.

zklapow and others added 5 commits May 9, 2020 14:43

canbus v1

0b8672e

use crate traits

e103a93

update crate name

e893bd9

correct can frame length

e495e35

cleanup warnings

b49010b

Sh3Rm4n requested changes May 10, 2020

View reviewed changes

zklapow added 3 commits May 16, 2020 20:03

add stm32f302x6/8 and CAN exmaple

2d761ae

fix PR comments and warnings

1a6ea4e

more cleanup

510e7a7

zklapow added 2 commits May 30, 2020 14:55

feature flag and cleanup

d402aa6

use arrayvec

2fa6fed

zklapow changed the title ~~[WIP] CAN Support~~ CAN Support May 30, 2020

Sh3Rm4n mentioned this pull request Jul 19, 2020

Add rtc module #93

Closed

Sh3Rm4n requested changes Jul 19, 2020

View reviewed changes

zklapow and others added 6 commits July 27, 2020 19:39

Update src/can.rs

eba0d39

Co-authored-by: Fabian <[email protected]>

Update Cargo.toml

be1a6fe

Co-authored-by: Fabian <[email protected]>

switch to heapless, cleanup from comments

4618c51

add some brief docs

bd0dd68

Merge branch 'master' into canbus-with-crate

d2595f5

Merge branch 'master' into canbus-with-crate

6d18d68

zklapow force-pushed the canbus-with-crate branch from 11ea850 to 6d18d68 Compare August 6, 2020 23:44

zklapow added 2 commits September 28, 2020 10:45

Merge branch 'master' into canbus-with-crate

df3203a

add CAN to changelog

5031371

Sh3Rm4n approved these changes Oct 18, 2020

View reviewed changes

zklapow added 4 commits October 18, 2020 22:40

Merge branch 'master' into canbus-with-crate

76efc71

release RX and TX

a73e28e

PR comments

a14c520

rename

3a22595

Sh3Rm4n merged commit 33c84a6 into stm32-rs:master Oct 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CAN Support #100

CAN Support #100

zklapow commented May 9, 2020 •

edited

Loading

Sh3Rm4n left a comment

Sh3Rm4n May 10, 2020

zklapow May 11, 2020

zklapow commented May 17, 2020

zklapow commented May 30, 2020

zklapow commented Jul 12, 2020

teskje commented Jul 15, 2020

zklapow commented Jul 17, 2020

Sh3Rm4n left a comment

Sh3Rm4n Jul 19, 2020

Sh3Rm4n Jul 19, 2020

zklapow commented Aug 6, 2020

David-OConnor commented Sep 27, 2020

zklapow commented Sep 28, 2020

teskje commented Sep 28, 2020

Sh3Rm4n left a comment

Sh3Rm4n Oct 18, 2020

zklapow commented Oct 19, 2020

Sh3Rm4n commented Oct 20, 2020


		static FILTER_INDEX: AtomicU8 = AtomicU8::new(0);

		pub struct Can {

	pub fn release(self) -> stm32::CAN {
	pub fn free(self) -> stm32::CAN {

CAN Support #100

CAN Support #100

Conversation

zklapow commented May 9, 2020 • edited Loading

Sh3Rm4n left a comment

Choose a reason for hiding this comment

Sh3Rm4n May 10, 2020

Choose a reason for hiding this comment

zklapow May 11, 2020

Choose a reason for hiding this comment

zklapow commented May 17, 2020

zklapow commented May 30, 2020

zklapow commented Jul 12, 2020

teskje commented Jul 15, 2020

zklapow commented Jul 17, 2020

Sh3Rm4n left a comment

Choose a reason for hiding this comment

Sh3Rm4n Jul 19, 2020

Choose a reason for hiding this comment

Sh3Rm4n Jul 19, 2020

Choose a reason for hiding this comment

zklapow commented Aug 6, 2020

David-OConnor commented Sep 27, 2020

zklapow commented Sep 28, 2020

teskje commented Sep 28, 2020

Sh3Rm4n left a comment

Choose a reason for hiding this comment

Sh3Rm4n Oct 18, 2020

Choose a reason for hiding this comment

zklapow commented Oct 19, 2020

Sh3Rm4n commented Oct 20, 2020

zklapow commented May 9, 2020 •

edited

Loading