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

Update documentation and smart-leds-trait dependency #34

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

SpamixOfficial
Copy link

Hello!

I've updated the documentation, since I had to search for hours myself to figure out that I had to add smart_leds_traits to make it work, as that wasn't covered in the documentation.

I've also updated the smart-leds-trait dependency to 3.0.0, so it's more up to date.

@SpamixOfficial
Copy link
Author

Just tested on a rp2040 board. Works very well 👍

@jannic
Copy link
Member

jannic commented May 9, 2024

Hi @SpamixOfficial!
As I understand the smart-leds / smart-leds-trait crates, this change shouldn't be necessary, as smart_leds::SmartLedsWrite should just be a re-export of smart_leds_trait::SmartLedsWrite.
What exact error do you get if you use the unmodified examples?
My guess is that the actual issue was a version mismatch, ie. you imported smart_leds v0.3.0, while ws2812-pio uses smart_leds v0.2.1.

@SpamixOfficial
Copy link
Author

Hello!

Yes, that was the issue if U remember correctly. The thing is: I would like to use the latest smart_leds if possible, since being on the latest version is always good practice. That's slaps why I created this PR :-)

@SpamixOfficial
Copy link
Author

Right and I also updated the examples as I could not get it to work at all when just using your example. I had to manually import smart_leds_traits 0.2.1 for it to work. That took a lot of time and forum searching to find out, so I also included some documentation updates in this PR!

Copy link
Member

@ithinuel ithinuel left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the contribution.
However, SmartLedsWrite is exported by smart-leds so the current code should work as is.

Wrt the bump of version to 0.3, please refer to #30's review (about supporting both versions).

@SpamixOfficial
Copy link
Author

Hi, thanks for the contribution. However, SmartLedsWrite is exported by smart-leds so the current code should work as is.

Wrt the bump of version to 0.3, please refer to #30's review (about supporting both versions).

Hello! Thanks for reaching back so quickly :-)

I'll take a look at this today, and hopefully this should be done soon!

// Alexander (Spamix)

@SpamixOfficial
Copy link
Author

@ithinuel quick question, doesn't IntoIterator also support Iterator? An update to 0.3.0 of smart-leds-trait shouldn't break any client code from my understanding?

@ithinuel
Copy link
Member

The issue wouldn't be on the iterator trait bond side.
[email protected]::SmartLedsWrite and [email protected]::SmartLedsWrite are two different traits (even if there's no change in between the versions).
So if we implement 0.3 but someone uses 0.2, things won't compile. (Because 0.3 and 0.2 are breaking changes in semver).

@SpamixOfficial
Copy link
Author

SpamixOfficial commented May 11, 2024

Oh, my bad haha!
Can't we just change the version number of this crate though? Like, I'm not sure what the current version is, but if we just as an example say the version of this crate is 0.2.x at the moment, we could change it to 0.3.x. That way no users who are dependent on the 0.2 version will be affected when it automatically updates, and everyone who wants to switch to a newer version of smart_leds will be able to!

Just an idea btw ^^

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.

3 participants