zxdavb / ramses_cc

HA integration for CH/DHW and HVAC systems that use the RAMSES II RF protocol
GNU General Public License v3.0
80 stars 17 forks source link

Add config flow #121

Closed trvrnrth closed 9 months ago

trvrnrth commented 11 months ago

Here's an example controller device page (ignore the unavailable entities, that's just a side-effect of my test data):

Screenshot 2023-12-23 at 15 18 16
zxdavb commented 11 months ago

Thanks. This is going to be amazing!

I have never had any time to get my head around config flow, so I'm not sure I can provide any value, other than testing, which will be when I get back home in a few days.

Adding tests would also be a good thing, but I am unsure how HACS supports such.

trvrnrth commented 11 months ago

testing, which will be when I get back home in a few days

There's no real rush on that at the moment as whilst the config flow here works, it's a long way off what it should be! I was planning on doing proper UI for the "standard" features (port selection, clearing cached schema/packets, performing discovery/eavesdropping) with the ability to specify the schema/known lists as yaml. And probably also free-form yaml to allow access to advanced ramses_rf features as well. I need to have a proper think about it.

Adding tests would also be a good thing, but I am unsure how HACS supports such.

I wouldn't imagine hacs would have any role in it to be honest. From what I've seen other components run their pytest suites standalone/with github actions. It's not an area I've investigated properly but it should be possible to crib from the core components and other third party components to set up a suitable framework. I agree that it would be good to have something in place. It will likely make sense to mock out ramses_rf unless it already has any suitable helpers.

zxdavb commented 11 months ago

I was planning on doing proper UI for the "standard" features (port selection, clearing cached schema/packets, performing discovery/eavesdropping) with the ability to specify the schema/known lists as yaml. And probably also free-form yaml to allow access to advanced ramses_rf features as well. I need to have a proper think about it.

I have had a few thoughts about this...

zxdavb commented 11 months ago

Port Selection:

ramses_rf needs to know if its utilizing an evofw3, or a HGI80 (other non-evofw3 gateways may exist, but have never been seen). If it has to, it assumes evofw3, which is a safe-enough bet.

Before that, it can sort to using the text in a by-id port name, so supplying:

... is preferable. It also means users can swap the device into another port without issue.

a) The challenge is that the underlying library used, pyserial, does not enumerate the by-id ports, and so neither does ramses_rf.

b) The opportunity is to auto-detect the port and so offer it as the only 'suggested' port, to the user during config flow.

Nonetheless, it has always been my intention to add a) and maybe b) to ramses_rf, so I'll do that now & let you know. It will be the same API for both.

See: https://github.com/pyserial/pyserial/pull/709/files

Finally, users may need to specify the baud rate under certain circumstances (the baud rate used to flash evofw3 on a NanoCUL).

FYI: in future the 'port' will be an MQTT endpoint.

zxdavb commented 11 months ago

Clearing cached schema/packets:

The challenge here is to have a scheme where the user can disable restoring one/both of these for the next restart of HA only.

Presently, users a) turn off cache, then b) restart HA, then c) have to re-enable the cache. It would be great if c) happened automatically.

That reminds me - I have meant to confirm that the old cache is wiped immediately when a user does a), then b), rather than wait for the next cycle of saving the latest cache.

If the user does a) & b) then c) and restarts HA before the next save of the store, they restore the old cache when not expecting to.

zxdavb commented 11 months ago

Performing discovery/eavesdropping:

Eavesdropping should be an advanced feature - say, only via YAML. The other ramses_cc advanced feature can be UI-enabled (send packets, message evenints).

zxdavb commented 11 months ago

It will likely make sense to mock out ramses_rf unless it already has any suitable helpers.

The ramses_rf test suite has very good mocking - basically a virtual RF network with virtual serial ports / RAMSES_II devices on it - ramses_rf can't tell the difference.

zxdavb commented 11 months ago

I have now modified the comports to include /dev/serial/by-id` symlinks:

from ramses_tx.transport import comports
   ...
ports = comports(include_links=True)
zxdavb commented 10 months ago

FYI, this is a similar UI to what we might end up with?

image

trvrnrth commented 10 months ago

FYI, this is a similar UI to what we might end up with?

Something along those lines should be possible. Co-incidentally I took delivery of a Zigbee radio yesterday so I'll be paying attention to how that config flow works when I get round to setting it up!

I've been mulling over how to put together a sane menu system for the various options but haven't had a chance to actually prototype anything yet.

zxdavb commented 10 months ago

Zigbee radio yesterday

I just got a HA SkyScanner (the serial dongle, above) - for matter - can't get it to appear in the Settings > System > Devices UI!

:(

trvrnrth commented 10 months ago

I have now modified the comports to include /dev/serial/by-id` symlinks:

from ramses_tx.transport import comports
   ...
ports = comports(include_links=True)

I haven't actually used this at present and instead normalise to a list by-id with get_serial_by_id following the approach taken in a few of the core components.

zxdavb commented 10 months ago

and instead normalise to a list by-id with get_serial_by_id following the approach taken in a few of the core components.

Well, I didn't know that!

trvrnrth commented 10 months ago

Opening for review as this should now be fully functional.

Will also require a branding PR to https://github.com/home-assistant/brands in order to add an icon for the integration. I don't know if you have anything suitable?

zxdavb commented 10 months ago

Opening for review as this should now be fully functional.

Will also require a branding PR to https://github.com/home-assistant/brands in order to add an icon for the integration. I don't know if you have anything suitable?

Is that still the case for when this integration is via HACS? I just don't know how that works - I never had to do it for the 3.5 integrations I wrote (it was before branding was a thing).

Or were you planning to get this integration into HA ?

I would have thought best place for it would have been official HACS integration.

Tell me your thoughts on that - I warn you that - IMO - it might be very difficult to get into HA.

zxdavb commented 10 months ago

I submitted an issue on my repo - but applies to this branch.

Also, observations:

They such great sensors, though - maybe best to ensure they're exposed ? ... dunno.

zxdavb commented 10 months ago

It looks fantastic !!

trvrnrth commented 10 months ago

Is that still the case for when this integration is via HACS? I just don't know how that works - I never had to do it for the 3.5 integrations I wrote (it was before branding was a thing).

The custom integrations (regardless of how they are sourced) live in https://github.com/home-assistant/brands/tree/master/custom_integrations

Or were you planning to get this integration into HA ?

I would have thought best place for it would have been official HACS integration.

Tell me your thoughts on that - I warn you that - IMO - it might be very difficult to get into HA.

I doubt it would be worth the effort to get it into core so wasn't something I was anticipating.

trvrnrth commented 10 months ago
  • heat demand of a zone should be a diagnostic sensor?

  • the TCS is called "Controller", but the DHW 'zone' doesn't have a name - it's heat demand should also be a diagnostic sensor?

They such great sensors, though - maybe best to ensure they're exposed ? ... dunno.

I believe you left a list of thoughts on those elsewhere. I have yet to take a proper look at that but I think it will make sense to do in a separate PR. The changes here make the entity category setup more evident but don't actually change them.

zxdavb commented 10 months ago

In order to add an icon for the integration. I don't know if you have anything suitable?

I don't know of anything suitable. It would have to be a generic icon.

RAMSES II applies to CH/DHW and HVAC, and is wireless. So pick anything you like, with that in mind.

zxdavb commented 10 months ago

BTW, do you have any appetite for adding config flow to the official evohome integration?

CONFIG_SCHEMA = vol.Schema(
    {
        DOMAIN: vol.Schema(
            {
                vol.Required(CONF_USERNAME): cv.string,
                vol.Required(CONF_PASSWORD): cv.string,
                vol.Optional(CONF_LOCATION_IDX, default=0): cv.positive_int,
                vol.Optional(
                    CONF_SCAN_INTERVAL, default=SCAN_INTERVAL_DEFAULT
                ): vol.All(cv.time_period, vol.Range(min=SCAN_INTERVAL_MINIMUM)),
            }
        )
    },
    extra=vol.ALLOW_EXTRA,
)

It has been on my to-do list for a very long time.

trvrnrth commented 10 months ago

BTW, do you have any appetite for adding config flow to the official evohome integration?

I'd probably describe it as limited but not necessarily non-existent.

zxdavb commented 10 months ago

I haven't had a chance to play with this as much as I'd have liked - will concentrate on getting faking working on 0.31.x for a short while.

Would you be OK to switch to python 3.12.x - there are some typing features with StrEnum I'd like to include (or is that mypy 1.8.x?)

trvrnrth commented 10 months ago

Would you be OK to switch to python 3.12.x - there are some typing features with StrEnum I'd like to include (or is that mypy 1.8.x?)

Oddly the core dev container hasn't been updated yet which I was waiting for to save myself some hassle. In the meantime though I've not noticed any typing/linting issues. Anyway, I'm sure I can sort out an upgrade when required so go ahead!

zxdavb commented 9 months ago

@trvrnrth

I am happy to pull this into my config_flow branch, however there are conflicts.

I was attempting to resolve the conflicts myself, but think I should leave it to you.

After that, I'll see if it starts without issue, and we can post it to HACS as a pre-release.

trvrnrth commented 9 months ago

@trvrnrth

I am happy to pull this into my config_flow branch, however there are conflicts.

I was attempting to resolve the conflicts myself, but think I should leave it to you.

After that, I'll see if it starts without issue, and we can post it to HACS as a pre-release.

I don't have time for any extensive testing this evening but I've merged in and confirmed that at a glance things look OK on my system.

zxdavb commented 9 months ago

Merged

zxdavb commented 9 months ago

@trvrnrth I will shortly post this as a pre-release, 0.40.0.

I would like to acknowledge your work - would you be OK if I ping you in the release notes, and/or on the HA forum?

trvrnrth commented 9 months ago

@trvrnrth I will shortly post this as a pre-release, 0.40.0.

Awesome.

I would like to acknowledge your work - would you be OK if I ping you in the release notes, and/or on the HA forum?

Yes, a mention is fine by me.