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

Update ZHA section about Zigbee OTA #28895

Open
wants to merge 10 commits into
base: current
Choose a base branch
from

Conversation

Hedda
Copy link
Contributor

@Hedda Hedda commented Sep 13, 2023

Proposed change

Update/improve section about Zigbee OTA/OTAU for the ZHA integration, including zigpy's new "Generic OTA providers" support.

References:

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.

Update section about Zigbee OTA for ZHA
@home-assistant home-assistant bot added the current This PR goes into the current branch label Sep 13, 2023
@home-assistant home-assistant bot added in-progress This PR/Issue is currently being worked on needs-rebase The PR has been branched of the wrong base branch or targets an incorrect target branch labels Sep 13, 2023
@home-assistant
Copy link

It seems that this PR is targeted against an incorrect branch since it has a parent PR on one of our codebases. Documentation that needs to be updated for an upcoming release should target the next branch. Please change the target branch of this PR to next and rebase if needed.

@Hedda
Copy link
Contributor Author

Hedda commented Oct 12, 2023

It seems that this PR is targeted against an incorrect branch since it has a parent PR on one of our codebases. Documentation that needs to be updated for an upcoming release should target the next branch. Please change the target branch of this PR to next and rebase if needed.

This is currently targeting current branch on purpose as it is only the ZHA documentation that is updated and none of the code.

Why is this tagged with "needs-rebase"? Is it because wrote PR fixes or closes issue: fixes # #28198 under additional information?

@Hedda Hedda removed their assignment Nov 24, 2023
Copy link

netlify bot commented Mar 11, 2024

Deploy Preview for home-assistant-docs ready!

Name Link
🔨 Latest commit e077a36
🔍 Latest deploy log https://app.netlify.com/sites/home-assistant-docs/deploys/6634f1b8cf23750008639da6
😎 Deploy Preview https://deploy-preview-28895--home-assistant-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@puddly puddly left a comment

Choose a reason for hiding this comment

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

Thanks! I think we want to shrink the size of this PR and remove content from the existing OTA docs, as a lot is no longer relevant.

Advanced features that are meant for developers should also be removed from the documentation. We want to keep integration documentation as short as possible. Otherwise, people are overwhelmed.

source/_integrations/zha.markdown Outdated Show resolved Hide resolved
source/_integrations/zha.markdown Outdated Show resolved Hide resolved
source/_integrations/zha.markdown Outdated Show resolved Hide resolved
source/_integrations/zha.markdown Outdated Show resolved Hide resolved
source/_integrations/zha.markdown Outdated Show resolved Hide resolved
source/_integrations/zha.markdown Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft March 12, 2024 19:38
@home-assistant
Copy link

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.

@Hedda
Copy link
Contributor Author

Hedda commented Mar 25, 2024

I tried to rewrite/update to meet the current feature-set but perhaps someone else would rather prefer to start over from scratch?

This was originally submitted in beginning of September 2023 and much/most has changed since, so maybe better to just close it?

@Hedda Hedda marked this pull request as ready for review March 25, 2024 09:33
@home-assistant home-assistant bot requested a review from puddly March 25, 2024 09:33
Copy link

@erkr erkr left a comment

Choose a reason for hiding this comment

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

This is a big improvement!

Copy link

@erkr erkr left a comment

Choose a reason for hiding this comment

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

@Hedda Just some minor remarks on the new OTA from a end user perspective.

One other remark. Reading various ZHA discussions revealed other options that are NOT mentioned in the new documentation. Are those options valid. If so how to use them:

  ota:
    # It's enabled by default
    enabled: true

    # Periodically send out a network-wide (including end devices!) image notification
    broadcast_enabled: true
    broadcast_initial_delay: 14400
    broadcast_interval: 14400

    # Points to Z2M OTA `index.json` if stored locally
    z2m_local_index: /path/to/z2m-ota/index.json

    # Points to Z2M OTA `index.json` URL
    z2m_remote_index: "https://raw.githubusercontent.com/Koenkk/zigbee-OTA/master/index.json"

source/_integrations/zha.markdown Outdated Show resolved Hide resolved
source/_integrations/zha.markdown Outdated Show resolved Hide resolved
@puddly
Copy link
Contributor

puddly commented May 3, 2024

Are those options valid. If so how to use them

OTA is meant to require no configuration and the defaults are carefully chosen. You don't have to do anything to enable it or use it. The options you've listed are meant for development or advanced users and are in the zigpy section of the config for that exact reason. If they are to be documented, it must be in the zigpy documentation, not ZHA's.

@Hedda Hedda marked this pull request as draft May 3, 2024 13:15
@erkr
Copy link

erkr commented May 3, 2024

I agree with the goal of no config😊. But due the lack of documentation it got hard to discriminate developer options from user options. So it was just question. Thanks for clarifying 👍

Update zha.markdown with OTA update rewrite
@Hedda
Copy link
Contributor Author

Hedda commented May 3, 2024

OK I have now given it a more serious attempt at a rewrite, including separating the UI part from the YAML configuration part.

50f9fe3

Please re-review this now.

Again, as mentioned above, I did not specifically mention that OTA for IKEA is disabled by default, but feel free to change that.

@Hedda Hedda marked this pull request as ready for review May 3, 2024 14:16
@cremor
Copy link

cremor commented May 3, 2024

I think it's a weird choice to document Ikea as supported, but don't document it as disabled by default. But if the devs want it that way then it's ok.
Other than that it looks good, thanks.

@erkr
Copy link

erkr commented May 3, 2024

My remarks are processed very well👍

@erkr
Copy link

erkr commented May 11, 2024

I think it's a weird choice to document Ikea as supported, but don't document it as disabled by default.

I saw your remark and wondered:

  • ZHA reports all my Ikea devices as up to date now. But I know several are really outdated.
    Is that because IKEA is disabled by default?
  • Is the reason because several issue are reports of failed updates with IKEA devices? That would be a valid choice, but then I agree with you to please mention that in the documentation!

@cremor can you shed a light on this please 🙏

@cremor
Copy link

cremor commented May 11, 2024

ZHA reports all my Ikea devices as up to date now. But I know several are really outdated.
Is that because IKEA is disabled by default?

I think so, yes.

Is the reason because several issue are reports of failed updates with IKEA devices? That would be a valid choice, but then I agree with you to please mention that in the documentation!

I don't know the reason, I'm not a ZHA dev. I just noticed that it's disabled by default:
https://github.com/zigpy/zigpy/blob/038d57b9430ebc9e0f9c1c106440f7d27161f4e7/zigpy/config/defaults.py#L26

@erkr
Copy link

erkr commented May 11, 2024

@cremor thanks!
Whatever the reason is to default IkEA by false, that should, IMHO, be documented.
If the reason is that it doesn't work, that is good to know

@Hedda
Copy link
Contributor Author

Hedda commented May 12, 2024

I think it's a weird choice to document Ikea as supported, but don't document it as disabled by default. But if the devs want it that way then it's ok.
Other than that it looks good, thanks.

Whatever the reason is to default IkEA by false, that should, IMHO, be documented.

Regardless, such information can be could be updated in a seperate pull request. I hate to see the merger of this PR delayed further due to debate over that. As it is this PR does not change the info about that in the ZHA docs so it should still be accepable to merge as-is, then someone else can create a new PR dedicated for that, which would also help clearify that specifically. That is, I do not think that discussion should be a showstopper for this specific pull request.

@erkr
Copy link

erkr commented May 12, 2024

I agree that publishing 'as is' is preferred above delay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
current This PR goes into the current branch in-progress This PR/Issue is currently being worked on needs-rebase The PR has been branched of the wrong base branch or targets an incorrect target branch
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

None yet

4 participants