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

Allow preserving WiFi credentials entered with captive_portal #3813

Merged
merged 5 commits into from
Oct 13, 2022

Conversation

kuba2k2
Copy link
Member

@kuba2k2 kuba2k2 commented Sep 15, 2022

What does this implement/fix?

This PR allows the user to preserve the WiFi credentials, that were entered using captive_portal, over subsequent OTA updates. I think this may be a useful feature, given that guides like Sharing ESPHome devices exist, and the recent addition of a "factory reset" component. When using keep_user_credentials, the user can freely reconfigure a device to a different network in a WiFiManager-like manner, whether it's a DIY, tuya-converted, or vendor-supplied ESPHome device.

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

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

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266
  • LibreTuya port

Example entry for config.yaml:

# Example config.yaml
wifi:
  keep_user_credentials: true
  # ssid: "my_ssid_unused"
  # password: "1234"
  ap:
    password: "ESPHome123"

Checklist:

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

Copy link
Member

@jesserockz jesserockz left a comment

Choose a reason for hiding this comment

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

The issue here is that if you flash a YAML over top that has credentials "built in", they wont work unless you remove the keep_user_credentials: true so there needs to be some validation on the config there to prevent both at the same time.

@kuba2k2
Copy link
Member Author

kuba2k2 commented Sep 15, 2022

Right, that's a good point! I'll refactor that tomorrow.

EDIT: Is a runtime (or compile-time) verification also required (in the C++ code)?

@kuba2k2 kuba2k2 marked this pull request as ready for review September 17, 2022 15:11
@kuba2k2
Copy link
Member Author

kuba2k2 commented Sep 17, 2022

Okay, the option is now incompatible with either ssid, password or networks.

Failed config

wifi: [source .\aubess-4b1-03h4-01.yml:35]

  WiFi credentials cannot be used together with keep_user_credentials.
  keep_user_credentials: True
  ssid: 127.0.0.1
  ap:
    password: FallbackApPassword

The corresponding esphome-docs PR has also been updated to make that clear.

@kuba2k2 kuba2k2 marked this pull request as draft September 23, 2022 09:33
@kuba2k2 kuba2k2 marked this pull request as ready for review October 9, 2022 09:57
@probot-esphome
Copy link

probot-esphome bot commented Oct 9, 2022

Hey there @OttoWinter, mind taking a look at this pull request as it has been labeled with an integration (captive_portal) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

Copy link
Member

@jesserockz jesserockz left a comment

Choose a reason for hiding this comment

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

Feels weird the way this works, but its fine.
Just a rename of the define below please.

esphome/components/captive_portal/__init__.py Outdated Show resolved Hide resolved
@jesserockz jesserockz merged commit 4bf94e0 into esphome:dev Oct 13, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Oct 15, 2022
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.

3 participants