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

bcm283x: Don't override previous pull states #13

Merged
merged 2 commits into from
Oct 16, 2021

Conversation

Reflejo
Copy link
Contributor

@Reflejo Reflejo commented Oct 12, 2021

Currently the pull resistors are being overridden when a new GPIO is set on the same byte space.

This case:

rpi.P1_12.In(gpio.PullDown, gpio.RisingEdge)
fmt.Printf("Pull State (Before) %s\n", rpi.P1_12.Pull())
rpi.P1_16.In(gpio.Float, gpio.NoEdge)
fmt.Printf("Pull State (After) %s %d\n", rpi.P1_12.Pull())

Will end up printing:

Pull State (Before) PullDown
Pull State (After) Float

The fix maintains the bits that are not being touched.

@codecov-commenter
Copy link

codecov-commenter commented Oct 12, 2021

Codecov Report

Merging #13 (7ec73f5) into main (d93ad22) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main     #13   +/-   ##
=====================================
  Coverage   33.9%   34.0%           
=====================================
  Files         49      49           
  Lines       5379    5381    +2     
=====================================
+ Hits        1826    1828    +2     
  Misses      3427    3427           
  Partials     126     126           
Impacted Files Coverage Δ
bcm283x/gpio.go 50.5% <100.0%> (+0.2%) ⬆️

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 d93ad22...7ec73f5. Read the comment docs.

@maruel
Copy link
Member

maruel commented Oct 12, 2021

gohci

@maruel
Copy link
Member

maruel commented Oct 12, 2021

Oh the errors are not your fault, you can ignore.
I want to double check something in the datasheet before committing but I think it's fine as is. It's impressive that this bug has lasted for so long.

@Reflejo
Copy link
Contributor Author

Reflejo commented Oct 13, 2021

@maruel I don't have a board to test the other branch but it looks like the legacy case is also broken

@berryboy2012
Copy link
Contributor

berryboy2012 commented Oct 14, 2021

Hi, nice work, BTW, could you please help fix some links in comments? I copied some discussion in slack to here for your convenience.

At #L409, https://github.com/raspberrypi/documentation/blob/master/hardware/raspberrypi/bcm2711/rpi_DATA_2711_1p0.pdf has been moved and revised to
https://datasheets.raspberrypi.org/bcm2711/bcm2711-peripherals.pdf (Pg. No. has also changed to around 65 and 73~76)
Also there may be other comments referencing the same broken link, for example #L1031. BCM283x has many quirks that an up-to-date datasheet is really helpful when debugging, thanks in advance!

Another question, could you please give some reference on libs talked here? thx!

NOTE: I'm not familiar with bcm283x but I did notice some python GPIO libraries are doing (previous & ~3) | .. instead. I'm not sure why it needs to clear up the two least significant bits

@berryboy2012
Copy link
Contributor

Another question, could you please give some reference on libs talked here? thx!

NOTE: I'm not familiar with bcm283x but I did notice some python GPIO libraries are doing (previous & ~3) | .. instead. I'm not sure why it needs to clear up the two least significant bits

nvm, https://github.com/RPi-Distro/raspi-gpio/blob/22b44e4765b4b78dc5b22394fff484e353d5914d/raspi-gpio.c#L889 seems to be a good reference, and dropping those two bits are necessary since we are doing a bitwise overwrite operation here.

@Reflejo
Copy link
Contributor Author

Reflejo commented Oct 14, 2021

Hi, nice work, BTW, could you please help fix some links in comments? I copied some discussion in slack to here for your convenience.

I rather keep this PR laser focus on this fix, can we do a new PR with the links fix?

@maruel
Copy link
Member

maruel commented Oct 16, 2021

Thanks for the change. It took a while because I wanted to confirm locally.

One tricky issue is that there's a subtle race condition, using an atomic compare exchange would get rid of it but it can be done as a follow up.

@maruel maruel merged commit deb1359 into periph:main Oct 16, 2021
@maruel
Copy link
Member

maruel commented Oct 16, 2021

I've updated the PDF reference in c025965.

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