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.

Sign up for GitHub

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

Jump to bottom

Feature/tlc594x driver #242

Merged
merged 2 commits into from
Jul 15, 2019
Merged

Feature/tlc594x driver #242

merged 2 commits into from
Jul 15, 2019

Conversation

linasdev
Copy link
Contributor

@linasdev linasdev commented Jul 14, 2019 •
edited
Loading

Finished the tlc594* driver and added it as a module (modm:driver:tlc594x). Tested and works with TLC5947.

salkinium
salkinium requested changes Jul 15, 2019
View reviewed changes
Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

Thanks for reviving this driver! I'm not sure why I didn't port it from xpcc in the first place, but your port is definitely an improvement!

Can you add an example for this driver? Then it's part of the CI and I will be reminded to refactor this too in case the APIs change.

src/modm/driver/pwm/tlx594x.lb Outdated Show resolved Hide resolved
src/modm/driver/pwm/tlc594x_impl.hpp Show resolved Hide resolved
src/modm/driver/pwm/tlc594x_impl.hpp Outdated Show resolved Hide resolved
src/modm/driver/pwm/tlc594x.hpp Outdated Show resolved Hide resolved
src/modm/driver/pwm/tlc594x_impl.hpp Outdated Show resolved Hide resolved
src/modm/driver/pwm/tlc594x.hpp Outdated Show resolved Hide resolved
@linasdev
Copy link
Contributor Author

Can I add an example for just one target?

@salkinium
Copy link
Member

Can I add an example for just one target?

Yes, I only expected one example anyways.

@linasdev
Copy link
Contributor Author

Added example for the stm32f103c8t6_blue_pill target.

salkinium
salkinium requested changes Jul 15, 2019
View reviewed changes
Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

Excellent! As a final polish, please also update the copyright of the driver files with the year and your name.

examples/stm32f103c8t6_blue_pill/tlc594x/main.cpp Outdated Show resolved Hide resolved
examples/stm32f103c8t6_blue_pill/tlc594x/project.xml Show resolved Hide resolved
@salkinium
Copy link
Member

I would also like to have your commits squashed into two commits:

  1. for adding the driver and updating the main readme.
  2. for adding the example and updating the example readme.

@linasdev
Copy link
Contributor Author

Squashed into two commits.

salkinium
salkinium previously approved these changes Jul 15, 2019
View reviewed changes
Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

Thank you very much!

@salkinium
Copy link
Member

Just fixed the commit messages, I forgot to say how we do it.

@salkinium salkinium merged commit b570d07 into modm-io:develop Jul 15, 2019
@linasdev linasdev deleted the feature/tlc594x_driver branch July 15, 2019 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@salkinium salkinium salkinium left review comments

Assignees
No one assigned
Labels
feature 🚧
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@linasdev @salkinium