Skip to content

Add documentation for device type option#12279

Merged
frenck merged 12 commits into
home-assistant:nextfrom
felipediel:patch-1
Apr 20, 2020
Merged

Add documentation for device type option#12279
frenck merged 12 commits into
home-assistant:nextfrom
felipediel:patch-1

Conversation

@felipediel
Copy link
Copy Markdown
Contributor

Proposed change

Add documentation for device type option.

Type of change

  • Spelling, grammar or other readability improvements (current branch).
  • Adjusted missing or incorrect information in the current documentation (current branch).
  • Added documentation for a new integration I'm adding to Home Assistant (next branch).
  • Added documentation for a new feature I'm adding to Home Assistant (next branch).
  • Removed stale or deprecated documentation.

Additional information

Checklist

  • This PR uses the correct branch, based on one of the following:
    • I made a change to the existing documentation and used the current branch.
    • I made a change that is related to an upcoming version of Home Assistant and used the next branch.
  • The documentation follows the Home Assistant documentation standards.

@frenck
Copy link
Copy Markdown
Member

frenck commented Mar 9, 2020

One question still remains:

Furthermore, how would a user, that reads this page, known which type to use?

How can a user find the 0x value? Which are available?

@felipediel
Copy link
Copy Markdown
Contributor Author

felipediel commented Mar 9, 2020

@frenck These are the types we know so far, but they get out of date very quickly, so I don't think it's interesting to document them.

The user can find the type of his device using the official Broadlink App. But I do not recommend this, because this application is evil. It connects the device to the cloud and prevents it from being controlled by Home Assistant. So the user should stay away from it.

The alternatives we have today are Broadlink Manager and this debug tool I've created to debug the API we are using. For now it's a pretty shitty tool, so I think we should stick to Broadlink Manager in the docs.

It is important to note that none of this would be necessary if we implemented HELLO_REQUEST (broadlink.discover) within our integration. The device knows its type and sends it in response. It would be so much easier to just say hello. We are violating etiquette and making the user pay for it.

frenck
frenck previously approved these changes Mar 10, 2020
Copy link
Copy Markdown
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

✅ Approved. Can be merged as soon as the parent PR gets merged.

@Quentame
Copy link
Copy Markdown
Member

Sorry for the immediate closing @felipediel but you will see later on that’s it’s annoying being requested for an unchecked code.

To prevent that, some advices :

  • always rebase, not merge (in general)
  • rebase just before creating a PR/MR
  • if you doubt you can create a draft PR
  • check the number of files impacted (and content)

And I don’t know if you have the right but PR can be reopened, or you can create a new one, so no worries 😉

Thanks a lot for your contribution 👍

Sent with GitHawk

@felipediel
Copy link
Copy Markdown
Contributor Author

@Quentame Yes, sorry about that. I was on a hurry late at night to send the docs and I worked on an old branch by mistake. I was so tired that I didn't even check my commits and the rest you know.
At the end of the day, that mistake was worth it, as I ended up learning how to do a decent rebase.
Thanks 👍

frenck
frenck previously approved these changes Apr 15, 2020
Copy link
Copy Markdown
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Nice! Thanks, @felipediel! 👍

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

Labels

has-parent This PR has a parent PR in another repo new-feature This PR adds documentation for a new Home Assistant feature to an existing integration next This PR goes into the next branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants