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

apa102: allow to specify SPI Mode in Opts #16

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

Conversation

alexgth
Copy link

@alexgth alexgth commented Aug 10, 2021

Some devices such as the secondary SPI Port of the Raspberry Pi 3 don't support the default SPI Mode 3.
This Change allows to set the SPI Mode to any other by changing the Opts.

@maruel
Copy link
Member

maruel commented Aug 20, 2021

Hi! Thanks for the contribution! Sorry I dropped it in my emails.

Frankly, I think I'd prefer to just switch to Mode0 and not make it an option. What is the downside?

@codecov-commenter
Copy link

codecov-commenter commented Aug 20, 2021

Codecov Report

Merging #16 (20c235b) into main (440ba6e) will not change coverage.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main     #16   +/-   ##
=====================================
  Coverage   59.8%   59.8%           
=====================================
  Files         48      48           
  Lines       4331    4331           
=====================================
  Hits        2590    2590           
  Misses      1608    1608           
  Partials     133     133           
Impacted Files Coverage Δ
apa102/apa102.go 93.8% <100.0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 440ba6e...20c235b. Read the comment docs.

@maruel
Copy link
Member

maruel commented Aug 20, 2021

Ignore the test failure, I need to redo the way I do forward-testing.

@alexgth
Copy link
Author

alexgth commented Aug 30, 2021

I'm not sure wether or not there might be some weird behaviour on the LED side of things due to the clock polarity being inverted. Whenever I accessed less LEDs than i connected the one LED after the last one was bright white, which might be because of the LEDs protocol (more likely for my understanding) or because of the SPI Mode. I unfortunately can't test it as I don't have access to the hardware anymore.

@sahib
Copy link

sahib commented Aug 30, 2021

@maruel: Are we sure that no other user of this library uses mode 3?
@alexgth: 👋

@maruel
Copy link
Member

maruel commented Aug 30, 2021

Frankly I'd simply use Mode1 and see if anyone complains on the next release. Users can always revert back to the previous release if it breaks them and we will add a bool flag at that point.

@sahib
Copy link

sahib commented Aug 31, 2021

@maruel: if users note that something is broken soon enough 😅 I guess the approach in this PR has the potential to save some tears. I was initially asking because I darkly recalled that we tried some LED strips initially on the first SPI port of a rpi3 (current approach uses second port), where mode 3 (old default) was working, but mode 1 not. That led to my guess that changing the default will break things for people.

@maruel
Copy link
Member

maruel commented Sep 12, 2021

Sorry for the delay, lots of things happening here.

Thanks for the data point about some LEDs not working with mode3. I'd still prefer to use a bool defaulting to false so that a constructing a Opt{} by hand still works. Right now with this change constructing a Opt{} will result in spi.Mode0, which doesn't work. Please document the flag to explain the specific use case, and the fact that some LEDs may not work when the bool is set to true.

I've finally fixed the github actions checks, so please git fetch and git rebase first. Thanks!

@maruel
Copy link
Member

maruel commented Oct 16, 2021

Revisiting this PR: I'd really prefer to use a bool instead of a spi.Mode; this way the default bool value in Go (false) is the right one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants