#Rainforest Eagle meter selection

1 messages ยท Page 1 of 1 (latest)

lean kraken
#

WDYT?

serene zodiac
#

I think it's weird if the integration replaces the source of the data without any notice, and continues to report using the same entities. I'd expect the new meter to generate a new device with connected entities, and we should make sure that the user can remove the old device and entities if wanted.

lean kraken
#

Oh so today it would always pick the first one. Even if it doesn't provide data

#

I'm not sure if they already create entities or just abort, will check in a moment

#

Current implementation adds the first from the list, regardless of connection status.
So today if you have a meter that once was factory reset, it would end you up with a config entry that has unavailable entities

serene zodiac
#

There's no stable identifier then for this device?

lean kraken
#

How do you mean

serene zodiac
#

I'm thinking if we add the form in the config flow where the user selects what meter to use, and then we'd make a reconfiguration flow, I think we need to make sure that the same entities aren't updated with the data of another meter. I'm assuming different meter's will report different data, even if connected to the same gateway.

lean kraken
#

Oh like that, yea I'll double check that

#

But more like, only a single meter can be attached at a time

serene zodiac
#

If we make sure that the entities aren't polluted with bad data from another meter, I think it sounds ok to allow the user to pick what meter to use, if more than one meter is available.

lean kraken
#

So from what I understand, there's only a single meter available at a time

serene zodiac
#

Even if only one meter can be connected at a time, I think we don't want to report data from different meters to the same entities during different time periods.

#

Preferably we should create a new device for every meter and separate entities per device.

lean kraken
#

Okay agreed on that one, just double checked

#

So today

#
    if meters is not None:
        if meters:
            hardware_address = meters[0].hardware_address
        else:
            hardware_address = None
#

It always takes the first option

#

But there can only be one connected at a time. So if you once factory reset your meter (which creates a new meter), you can't use the integration anymore

#

So the change now is, do we want to just pick the first that's connected and working at this place, or do we ask the user

#

(currently I don't think we really do the unique id uniqueness correctly, but that's something we can look at)

serene zodiac
#

For new config entries, I think it's ok to automatically connect to the meter that is connected. In a reconfiguration flow I'd like some more context about the current meter (that may not be working anymore) and what other meter we're going to connect to or what meter we succesfully connected to. I think that's important since changing meter will mean a new device and new entities, and automtains would need to be updated etc.

lean kraken
#

Yea I think reconfiguration should only happen to the same meter, not to a different one

serene zodiac
#

Right, that's easier.

carmine hill
#

Hey, guys, I'm the owner of the PR and just wanted to clarify:
Doing a factory reset does not inherently create another meter in the list. If you do a reset and then reconnect with the same electric meter, it will continue to be the first and only in the list. However, if your power company swaps out your meter for another one, or you move to a new home, performing a reset will not clear the list, so now the first one is always disconnected and the second one is always what you want in that case.
I totally support just always using the first one in the list with a "connected" status. The only reason I went the direction of "give the user a choice" was because of the mentioned PR from a few years ago being rejected because it didn't give the user a choice. I'm happy to change the PR back to what was originally suggested if that's the desired direction at this point.

#

Sounds like the most important part initially would be putting the hardware ID of the specific meter on the entity IDs to avoid mixing historical data between different meters? I can look at that first

lean kraken
#

Yes that'd be great! I'd opt for trying to do this automatically as I don't really see the user making a different choice, and thus we make it a bit easier, both for the users and for ourselves as we have less code to maintain

carmine hill
#

I haven't had a good chance to make any changes yet, but yea, if everybody is in agreement with that, I'll change to that approach

carmine hill
#

hey, guys, I finally had a chance to give this a go and ended up taking a bit of a hybrid approach to what was suggested, so I'm curious what you think:

  1. If there's only one meter in the list, use it
  2. If there's multiple meters in the list, use the first connected one
  3. If there are multiple meters in the list but none are connected, show the list of meters, with hardware IDs and connection statuses, to let the user choose one.

In all cases, I added the meter's hardware address to the entity IDs. Interestingly enough, it was actually already included in _attr_unique_id, so I believe the original concern about conflicting entities may have already been at least somewhat inherently resolved by that, but this at least makes it more apparent. I also learned that if you don't delete the device/entity registries between tests, it will just keep using the old entity ID every time and make you bang your head on the desk for an hour ๐Ÿคฆโ€โ™‚๏ธ ...but I learned eventually.

Let me know what you think!

lean kraken
#

If there are multiple meters in the list but none are connected
Would it make sense to just tell the user that somethign is wrong instead and abort?

carmine hill
#

If there's only one meter and it's not connected, it would continue without error and then the sensors would just all be "unavailable", so I feel like having multiple meters should allow getting to the same state with any of them, but I don't really have any good use-cases to go off of.

lean kraken
#

I mean if there's only one and it's disconnected I'd maybe argue the same

#

As in, the config flow is the most user friendly way to say something is wrong

#

Like it's more weird if it showed everything worked and then it doesn't set up

carmine hill
#

Yea, that's fair...that's basically the situation I was in when I connected up my new meter. Setup worked fine and just always showed "unavailable". I can get behind universally requiring a meter to be connected during setup, that's fine.

lean kraken
carmine hill
#

of course my electric company's webpage for connecting/disconnecting smart meters is broken tonight, so I can't really do a true manual test of disconnected meters...I was trying to just get it out of range of the meter while remaining plugged into ethernet, but even wrapped in foil, in a faraday bag, in my safe, on the opposite side of the house, it was still somehow reading data off the meter ๐Ÿคฏ . That zigbee signal is stronger than I thought!

lean kraken
#

Hehe

#

Given that we know how connect disconnect work i think id be fine without manual test

carmine hill
#

yea, I'll fake it to see the error and then trust that things are doing what they're supposed to and push here in a bit

#

this is what the entity IDs are looking like

lean kraken
#

Fyi, entity ID and unique id is different

#

I think we could make this entity ID prettier, because it probably means we add the 0x0.. to the device name, or that we assign a custom entity ID

carmine hill
#

currently assigning a custom entity ID and the 0x just comes straight through from the hardware address. I was considering just using the last 8 characters or something a little less obnoxious. Open to suggestions there.

lean kraken
#

Did we always assign the entity is

#

Id

carmine hill
#

nope, it was using the automagic of the device name and translate_key

#

should I be putting the hardware address on the device name, instead?

lean kraken
#

I'd say depending on the amount of entries people have in general

lean kraken
#

Setting entity ID ourselves now loses translations etc

carmine hill
#

here's my faked error test. Just looking for "Cheese" instead of "Connected".

carmine hill
#

that's all pushed and good, now, but I didn't make any further changes to the entity ID stuff, so let me know if you have any suggestions/requests for that and I'll try to get that wrapped up tomorrow

lean kraken
#

Awesome ๐Ÿ™‚

#

Looks like we can merge this before tomorrow

#

then we can get it in 2026.4

carmine hill
#

That would be fantastic! I'll be around all day today if there are any requests for entity IDs, like dropping the 0x, using a subset of the characters, making the whole device unique to the meter, etc, but otherwise I won't touch it, in case we're good with how it is.

lean kraken
#

@carmine hill just checking in on the tests, I see that one of the tests does

with (
        patch(
            "homeassistant.components.rainforest_eagle.config_flow.async_get_type",
            return_value=(TYPE_EAGLE_200, []),
        ),

and then ends up with

    assert result2["data"] == {
        CONF_TYPE: TYPE_EAGLE_200,
        CONF_HOST: "192.168.1.55",
        CONF_CLOUD_ID: "abcdef",
        CONF_INSTALL_CODE: "123456",
        CONF_HARDWARE_ADDRESS: None,
    }

No hardware address

#

How does that work?

carmine hill
#

No hardware address is valid for the Eagle-100 hardware, I believe (I don't have that hardware to test it specifically, but I assume it's just kind of blind to what meter is connected and passes data through). However I should have made that test uses the EAGLE-100 type to be more explicitly clear there. I'll change that when I'm off work in a couple hours

lean kraken
#

I can do that right now

#

but I think its still weird that the integration is just able to set up with the 200 with no meters

carmine hill
#

Yea, that's a good point. I can't remember if there is something in place earlier in the flow in the existing code that would prevent that

lean kraken
#

In any case, I made a few tweaks, like making the tests finish etc, will push now

carmine hill
#

Awesome, thanks!

#

data.py line 51 is what I was thinking of. When meters is None it follows a different path for the Eagle-100, so the Eagle-200 path could probably stand to have checks or improvements for None or empty list and error in those cases.

#

I'm just looking on my phone, though, so I didn't poke through all of that

carmine hill
#

I take it we didn't quite make the cut? ๐Ÿ˜ข I'll take another look at the empty list/None stuff tonight and see what you pushed

lean kraken
#

Oh yea we did not

#

But Friday is 2026.4.1

#

And next Friday .2

carmine hill
#

Right right, I'll try to get that buttoned up and hope for Friday

lean kraken
#

I think this was the last one

#

Hence I already pulled the code and wanted to give it a last push

#

But then i saw this and was like, let me double check

#

Also, side question, ever considered to become codeowner ๐Ÿ˜‰

carmine hill
#

Ohhhh boy, I don't think I have the time to commit to that, but I suppose it doesn't seem to be an overly-popular integration, so potentially... Are the existing code owners really as inactive as it seemed? Seems like there would have to be a better fit considering this is my first contribution to any of HA ๐Ÿ˜†

lean kraken
#

But it's always nice and useful to just have someone around who's willing to help with an issue here or there or has access to a device to test things

#

You don't want to know how many times we didn't know if something was returned as a string or an int because nobody had such device

carmine hill
#

Yea, that's fair...I'm open to it

lean kraken
#

Or you could look at the quality scale and check if we use all the features it could use

carmine hill
#

I believe these things can also do zigbee stuff, so I'm guessing there is additional functionality available there that we could use but probably not very beneficial since people generally have a dedicated zigbee dongle

lean kraken
#

I mean i think its just a part Exploring, what do you want to get out of it

#

Sometimes i implement things because i can

#

But not spending a ton of time

carmine hill
#

alright, I'm calling it a night, but I got everything pushed...I ended up rolling things back a bit further to play it safer (KISS, right?), basically taking the same approach as the old PR from years ago by doing the 'Connected' check within async_get_type() and returning the hardware address there, instead of the list of meters. This cleaned things up quite a bit, reduced the amount of changes, and felt like a safer bet for avoiding any regressions. I did, however, add some checks for the unexpected cases of None and unsupported device types and then updated and added tests accordingly, with mocks of the EAGLE requests, instead of the async_get_type(), since the logic for finding the connected meter moved.

Hopefully that makes everything clearer and safer and easier to land, but if you were in support of the refactoring to move the 'Connected' logic up a level, just let me know and I'll shift it back to there.

#

ohp, and right after I say that, I see this...is there a new test coverage requirement? Guess I'll revisit this tomorrow, if that's the case

carmine hill
#

All better!

carmine hill
#

Thank you! ๐Ÿค˜