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

Add optional target IP parameter to setup() #619

Closed
wants to merge 1 commit into from

Conversation

IgitBuh
Copy link

@IgitBuh IgitBuh commented Oct 5, 2021

Allows providing a target IP for setting up wifi credentials instead of broadcasting them.

See issue #618 for details.

Allows providing a target IP for setting up wifi credentials instead of broadcasting them.
@felipediel
Copy link
Collaborator

Hi @IgitBuh. Thanks for working on this!

Please...

  1. Rename "target_ip" to "ip_address"
  2. Use the constant DEFAULT_BCAST_ADDR instead of "255.255.255.255".
  3. Create the PR against the dev branch.

@IgitBuh
Copy link
Author

IgitBuh commented Oct 18, 2021

1. Rename "target_ip" to "ip_address"

My intention was to make it clear that in contrast to other IP parameters like in hello() this is not the local IP but the target IP. Maybe target_ip_address then? I actually don't mind either way, I just wanted to point this out.

2. Use the constant DEFAULT_BCAST_ADDR instead of "255.255.255.255".

Are you sure? It is currently used multiple times like this. If you want to change it to a constant, you should probably change all occurrences. I'm not even sure where DEFAULT_BCAST_ADDR is defined.

Would you mind doing these changes yourself to your liking? I don't have the Broadlink device with me right now and cannot test any modifications.

@felipediel
Copy link
Collaborator

Ok, done.

@felipediel felipediel closed this Oct 18, 2021
@felipediel felipediel mentioned this pull request Oct 18, 2021
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants