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

Fix pin reuse error with pin expanders #5973

Merged
merged 4 commits into from
Dec 20, 2023
Merged

Fix pin reuse error with pin expanders #5973

merged 4 commits into from
Dec 20, 2023

Conversation

jesserockz
Copy link
Member

What does this implement/fix?

If the same pin number was used but from two different i/o expanders in the same platform, the error was given. This changes to use the id of the expander as part of the key

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other

Related issue or feature (if applicable): fixes

Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266
  • RP2040
  • BK72xx
  • RTL87xx

Example entry for config.yaml:

# Example config.yaml

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

@jesserockz jesserockz added this to the 2023.12.0b6 milestone Dec 20, 2023
@jesserockz jesserockz requested a review from a team as a code owner December 20, 2023 08:24
kbx81
kbx81 previously approved these changes Dec 20, 2023
@clydebarrow
Copy link
Contributor

Since the ID will be unique for each instance of a component, shouldn't it be sufficient to just use the changes around line 68 to use the correct value for the first part of the key, rather than adding an extra dimension to the key?

That section of code (lines 68 and 69) did bother me at the time since it appeared to work, but I was not 100% clear on why.

@clydebarrow
Copy link
Contributor

Or is it possible that a single component can have multiple registered pin schemas? In which case the extra key dimension is needed.

@jesserockz
Copy link
Member Author

Since the ID will be unique for each instance of a component, shouldn't it be sufficient to just use the changes around line 68 to use the correct value for the first part of the key, rather than adding an extra dimension to the key?

That section of code (lines 68 and 69) did bother me at the time since it appeared to work, but I was not 100% clear on why.

I tried that, but the key is pulled out later on line 107 to get the final_validation function, so the non-id key is needed

@clydebarrow
Copy link
Contributor

Ah ok. Looks Good Enough to me. I'm sure it will get revisited over time :-)

esphome/pins.py Outdated Show resolved Hide resolved
@esphome esphome bot marked this pull request as draft December 20, 2023 10:20
@esphome
Copy link

esphome bot commented Dec 20, 2023

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@jesserockz jesserockz marked this pull request as ready for review December 20, 2023 10:23
@esphome esphome bot requested a review from clydebarrow December 20, 2023 10:23
@jesserockz jesserockz enabled auto-merge (squash) December 20, 2023 10:26
@jesserockz jesserockz merged commit 84174ae into dev Dec 20, 2023
49 checks passed
@jesserockz jesserockz deleted the jesserockz-2023-438 branch December 20, 2023 10:42
@jesserockz jesserockz mentioned this pull request Dec 20, 2023
jesserockz added a commit that referenced this pull request Dec 20, 2023
Co-authored-by: Clyde Stubbs <2366188+clydebarrow@users.noreply.github.com>
@justlikeef
Copy link

Confirmed this fixed the issue for me in dev

@jesserockz jesserockz mentioned this pull request Dec 20, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants