Skip to content

Select number of phases setting - #101

Open
philipostli wants to merge 3 commits into
tibber:mainfrom
philipostli:select-number-of-phases-setting
Open

Select number of phases setting#101
philipostli wants to merge 3 commits into
tibber:mainfrom
philipostli:select-number-of-phases-setting

Conversation

@philipostli

Copy link
Copy Markdown

Why

On a 1-phase power-grid we do not need the two empty phases measuring no current, so I wanted a setting to hide them from capabilities view.

What

I added a checkbox setting to the Pulse and Watty drivers
Default value is true (most people have 3-phases)
Upon change it removes/adds the capabilities measure_current.L2 and measure_current.L3 on the device

Test

Tested on Pulse. ✔️
Watty not tested, but should work similarly.

@philipostli
philipostli requested a review from a team October 30, 2024 15:13
@jstaro

jstaro commented Nov 18, 2024

Copy link
Copy Markdown
Member

I'll have to double check, but I'm fairly certain it is not guaranteed that L1 will be the reporting phase on a 1-phase grid.

@philipostli

Copy link
Copy Markdown
Author

I see. Then lets check to see what phases have no value, and hide them. Then show all three when setting is switched back. Will work on rare 2-phase installations as well.

@jstaro

jstaro commented Apr 3, 2025

Copy link
Copy Markdown
Member

@philipostli
Sorry for the very late reply.
We can indeed not guarantee L1 to be the reporting phase since we just forward what we get from the meter.
Detecting this might sound easy, but we'd need to do it over a time window since datagrams don't always report values in all registers, and we are not sure the value-add is big enough to warrant a heuristic that might become a source of bugs.

So we think this needs to be 3 settings to hide/show individual phases. Not sure it's worth the boilerplate for this. Since I myself do not have a 1-phase installation, how big of a nuisance is this in the app?

@philipostli

Copy link
Copy Markdown
Author

Thanks for replying. I do not see the need for time tracking the value. My capability dashboard says '-' in those two other phases, and the old value is there on app-restart. If we switch on the setting, we instantly know what phase to show and what two others to hide. If this do not work somehow, one can always switch it of again to reveal all three phases. But for the purpose of POC I also do exactly this same thing in the Zaptec app that I develop together with Patrick. I can not see how it will introduce bugs. As long as one always check if a capability exists:

if (device.hasCapability('capability_name')) {
    device.setCapabilityValue('capability_name', value);
}

I now have 8 capabilities and have no need to scroll. But adding only one more will make it a bit annoying. I find it annoying just now with two unused capabilities.

@jstaro

jstaro commented Apr 28, 2025

Copy link
Copy Markdown
Member

@philipostli
I think to do this somewhat safely without breaking people (since the app has always shipped with the capabilities and not added them dynamically) would be to have individual settings per phase, default unchecked:

  • Disable L1 Phase capabilities
  • Disable L2 Phase capabilities
  • Disable L3 Phase capabilities

And then, in onInit, do something like

if (this.getSetting('disable_capabilities_phase_l1')) {
    if (this.hasCapability('measure_current.L1')) {
        await this.removeCapability('measure_current.L1');
    }
} else {
    if (!this.hasCapability('measure_current.L1')) {
        await this.addCapability('measure_current.L1');
    }
}

// etc...

If we were to do this.removeCapability without any additional checks for all three and then start over from a clean slate by adding them dynamically as they show up in the data, it would remove all flow cards that use any of the capabilities and that is not good.

Using the plural disable_capabilities_phase_l1 in case we want to add another setting to include voltage capabilities as mentioned in this use case.

Thoughts?

@philipostli

Copy link
Copy Markdown
Author

I understand your concerns. I tested alot to learn about the implications of removing capabilities. I have found four points of interests:

  1. The built in flowcards connected to the measure_current capability will be invalid, throwing error.
  2. Tokens used in flows will also throw error on the card they are used.
  3. And the indicator on the dashboard tile would be reset, if the removed capability was the one chosen.
  4. Re-adding the capability would place it in the bottom in the capabilities dashboard

Now I guess people have fixed number of phases. And have not used flowcards for the phases that have no value in them? If not, I guess the risk of breaking flows is minimal.

But as an alternative one could just hide the capability in the dashboard by changing the ui-component, dynamically:

async onSettings(changes) {
  const phases = ["L1", "L2", "L3"];
  
  for (const phase of phases) {
    const settingKey = `disable_capabilities_phase_${phase}`;
    
    if (changes.changedKeys.includes(settingKey)) {
      const hide = changes.newSettings[settingKey];
      
      // Capabilities for denne fasen
      const phaseCapabilities = [
        `measure_current.${phase}`,
        `measure_voltage.${phase}`
      ];
      
      for (const cap of phaseCapabilities) {
        if (this.hasCapability(cap)) {
          await this.setCapabilityOptions(cap, {
            uiComponent: hide ? 'none' : 'sensor'
          });
        }
      }
    }
  }
}

This would keep all the flowcards, tokens etc, and keep the ordering of the capabilities dashboard.

Migrating the capability on init would be unneccesary. Because the device keeps its capabilities upon restarts. And new devices have them all enabled by default. We would only need the code in onSettings().

…ce single phase mode checkbox with individual checkboxes for each phase (L1, L2, L3) in the configuration. Update device logic to handle visibility changes accordingly.
@philipostli

Copy link
Copy Markdown
Author

Lets do it simple and just hide the ui-component. Not fiddling with the capability more than necessary.

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