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

feat: long range support [email protected] #3545

Merged
merged 37 commits into from
Apr 3, 2024
Merged

Conversation

robertsLando
Copy link
Member

@robertsLando robertsLando commented Jan 22, 2024

Fixes #3536

  • Add long range keys to settings
  • Add protocol property to nodes and supportsLongRange to controller (also shown on ui dedicated column)
  • Allow to specify protocol on provisioning entries add/edit
  • Allow to specify protocol during grant security step on nodes manager dialog

Copy link
Member

@AlCalzone AlCalzone left a comment

Choose a reason for hiding this comment

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

  • Z-Wave protocl Icon tooltip should be "Z-Wave", not "ZWave"
  • ZWLR protocol icon tooltip should be "Z-Wave Long Range", not "ZWaveLongRange"
  • After scanning a QR code with ZWLR in the supportedProtocols, the provisioning entry shouldn't be active immediately, otherwise the user may not have time to switch to ZWLR
  • Provisioning entries with ZWLR support don't let the user change protocol. This must be possible
  • When changing a provisioning entry to ZWLR, S2 Unauthenticated and S0 Legacy should be disabled and deselected. ZWLR doesn't have these classes
  • Hide node neighbors on the network map for ZWLR nodes
  • Hide route management on the network map for ZWLR nodes
  • Hide "rebuild routes" on the network map for ZWLR nodes
  • Diagnose/Health check for ZWLR nodes: disable all other targets except the controller
  • This must not be done for ZWLR:
    // in ZwaveClient -> _onNodeReady:
    if (!zwaveNode.isControllerNode) {
        this.getPriorityRoute(zwaveNode.id).catch((error) => {
            this.logNode(
                zwaveNode,
                'error',
                `Failed to get priority route for node ${node.id}: ${error.message}`,
            )
        })
    
        this.getCustomSUCReturnRoute(zwaveNode.id)
        this.getPrioritySUCReturnRoute(zwaveNode.id)
    }

@AlCalzone
Copy link
Member

  • This must not be done for ZWLR:
    ```js
    // in ZwaveClient -> _onNodeReady:
    if (!zwaveNode.isControllerNode) {
    this.getPriorityRoute(zwaveNode.id).catch((error) => {
    this.logNode(
    zwaveNode,
    'error',
    Failed to get priority route for node ${node.id}: ${error.message},
    )
    })

        this.getCustomSUCReturnRoute(zwaveNode.id)
        this.getPrioritySUCReturnRoute(zwaveNode.id)
    }
    ```
    

@robertsLando I meant the entire block. There are also no priority routes to ZWLR nodes. It's always direct connection. getPriorityRoute returns no route.

Copy link
Member

@AlCalzone AlCalzone left a comment

Choose a reason for hiding this comment

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

See previous comment and:

  • Do not request node neighbors for ZWLR nodes
  • Activating provisioning entry for ZWLR does not work - it's immediately disabled again
  • Editing popup is out of sync with provisioning list
  • When changing a provisioning entry to ZWLR, S2 Unauthenticated and S0 Legacy should be disabled and deselected. ZWLR doesn't have these classes
  • The advanced dialog for ZWLR nodes must not have the "rebuild routes" action
  • The nodes properties dialog for ZWLR nodes looks like the one for the controller now, instead of having statistics, ping, diagnose actions.

@robertsLando
Copy link
Member Author

I meant the entire block

Yeah missed that, fixed

@coveralls
Copy link

coveralls commented Jan 24, 2024

Pull Request Test Coverage Report for Build 8538120783

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 2 of 147 (1.36%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 22.392%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/stores/base.js 0 8 0.0%
src/lib/items.js 0 12 0.0%
src/lib/utils.js 0 23 0.0%
src/components/nodes-table/nodes-table.js 0 41 0.0%
api/lib/ZwaveClient.ts 0 61 0.0%
Totals Coverage Status
Change from base Build 8449004279: -0.2%
Covered Lines: 3896
Relevant Lines: 18481

💛 - Coveralls

@robertsLando
Copy link
Member Author

This can be tested using docker test tag

@AlCalzone
Copy link
Member

Only beta releases for this please. I can't support potential breaks yet.

@robertsLando
Copy link
Member Author

@AlCalzone Yeah no worries until this is not merged no official release

@AlCalzone
Copy link
Member

👍🏻 I first saw the comment about you missing the release, then this PR (which I now notice I've seen before), so I got spooked.

@robertsLando robertsLando merged commit bbf5ee6 into master Apr 3, 2024
4 of 8 checks passed
@robertsLando robertsLando deleted the feat-long-range branch April 3, 2024 11:46
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.

Add support for long range (inclusion choice, routing changes, etc)
3 participants