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

Small fixes for CAN #186

Merged
merged 7 commits into from
Mar 7, 2021
Merged

Small fixes for CAN #186

merged 7 commits into from
Mar 7, 2021

Conversation

zklapow
Copy link
Contributor

@zklapow zklapow commented Dec 20, 2020

I know there is an open issue for refactoring the CAN impl (#178) but figured I would patch the existing impl slightly in the meantime.

This fixes an issue where weren't actually transmitting the correct length of data, as well as adding options to allow users to configure the baud rate and enable loopback mode.

@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Dec 20, 2020

Don't worry about #178 too much. This is still very WIP and for now only contains added TODO comments. More of a reminder for myself. Maybe I should close that PR until it contains some useful changes.

src/can.rs Outdated Show resolved Hide resolved
src/can.rs Outdated Show resolved Hide resolved
@zklapow
Copy link
Contributor Author

zklapow commented Jan 10, 2021

updated this to use a builder + the source enums where possible. Sorry for the delay havent had much time to work on this!

@zklapow
Copy link
Contributor Author

zklapow commented Jan 16, 2021

(also didnt realize the clippy lints were failing, fixed now)

Copy link
Member

@Sh3Rm4n Sh3Rm4n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long delay on my side again. I've to figure out to devote some time regularly to this project. The last active maintenance efforts of myself are reduced to short bursts.

Some very minor nits, the rest looks ok to me. As Can::new() has the same API as before, this is not API break. Changelog entries are always welcome, but in this case not required.

In the long run, we should think about using the https://github.com/stm32-rs/bxcan types,
and in the longer run, see if we can copy / merge some ideas from stm32-rs/stm32f1xx-hal#293.

But this takes time. As of now, I'm really happy, that you are doing this work on the CAN peripheral right now! :)

src/can.rs Show resolved Hide resolved
src/can.rs Outdated Show resolved Hide resolved
@zklapow
Copy link
Contributor Author

zklapow commented Mar 3, 2021

no prob @Sh3Rm4n I am in the same boat myself. I find occasional bouts of time to work on this, but much less than I would prefer!

I will make some of those changes later today and update the changelog as well.

I totally agree about using bxCan, I was looking for something like that to exist when I originally started work on this! I have been meaning to give that a go, but have been hesitant to mess with it because I have been using the existing CAN stuff for a motor driver I am building and didn't want to mess with it. I might give it a go soon though.

@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Mar 3, 2021

I will make some of those changes later today and update the changelog as well.

Nice 👍

I totally agree about using bxCan, I was looking for something like that to exist when I originally started work on this!

Yeah, I know that your original work on CAN predates the bxCAN crate. It's nice to see the eco-system improving :)

I have been meaning to give that a go, but have been hesitant to mess with it because I have been using the existing CAN stuff for a motor driver I am building and didn't want to mess with it.

No pressure. Maybe some else has time to pick this up and you can focus on your motor driver! :)

Copy link
Member

@Sh3Rm4n Sh3Rm4n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Do want to add anything or can I merge it?

@zklapow
Copy link
Contributor Author

zklapow commented Mar 5, 2021

Should be good to merge!

@Sh3Rm4n Sh3Rm4n merged commit 9e29ec6 into stm32-rs:master Mar 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants