home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.
https://www.home-assistant.io
Apache License 2.0
70.47k stars 29.4k forks source link

ModBus hub declaration now requires at least an entity to work, otherwise `timeout is adjusted(-1) due to scan_interval` #112591

Closed JohnKiller closed 5 months ago

JohnKiller commented 5 months ago

The problem

I have declared a modbus configuration without any entity:

modbus:
  - name: "HAIER-AC"
    delay: 1
    type: serial
    baudrate: 9600
    bytesize: 8
    method: rtu
    parity: N
    port: /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_AB0PI859-if00-port0
    stopbits: 1

I then use this bus with a custom integration. Prior to 2024.3.0 this worked. Now I got this warning:

Modbus HAIER-AC timeout is adjusted(-1) due to scan_interval

After that:

Unexpected error fetching HAIERAC ModBus data: Not a valid timeout: -1

This is caused by https://github.com/home-assistant/core/blob/a92e65bc54a4ce2126bf703a97266cf688ec1599/homeassistant/components/modbus/validators.py#L365-L397

The code declares minimum_scan_interval = 9999 only inside the for loop. I have no entities, so the code never enters that.

What version of Home Assistant Core has the issue?

2024.3.0

What was the last working version of Home Assistant Core?

2024.2.5

What type of installation are you running?

Home Assistant OS

Integration causing the issue

ModBus

Link to integration documentation on our website

https://www.home-assistant.io/integrations/modbus/

Diagnostics information

No response

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

No response

home-assistant[bot] commented 5 months ago

Hey there @janiversen, mind taking a look at this issue as it has been labeled with an integration (modbus) you are listed as a code owner for? Thanks!

Code owner commands Code owners of `modbus` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign modbus` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

(message by CodeOwnersMention)


modbus documentation modbus source (message by IssueLinks)

janiversen commented 5 months ago

That is not a bug in the modbus integration. please talk with the developers of the custom integration.

In the modbus integration this is an important warning message, which is correct.

The error message is caused by the custom íntegration and have to be solved there.

JohnKiller commented 5 months ago

Sorry, maybe I've put my words wrong.

Discard for a moment the custom integration (it's mine). Let's talk just about this file: modbus/validators.py

At line https://github.com/home-assistant/core/blob/a92e65bc54a4ce2126bf703a97266cf688ec1599/homeassistant/components/modbus/validators.py#L389-L390

the variable minimum_scan_interval is defined here:

https://github.com/home-assistant/core/blob/a92e65bc54a4ce2126bf703a97266cf688ec1599/homeassistant/components/modbus/validators.py#L376

that is never reached if this for is empty:

https://github.com/home-assistant/core/blob/a92e65bc54a4ce2126bf703a97266cf688ec1599/homeassistant/components/modbus/validators.py#L371-L373

and is what is happening here because I have no entities. If in the config I put a dummy entity, then it starts working again.

I think this is a bug in the core, not in my custom integration.

Thanks anyway for your time.

janiversen commented 5 months ago

Why would you ever configure the modbus integration without any entities....that makes no sense (apart from when you use it with a custom_component)...so that is an unsupported configuration !

Actually you found a bug in the modbus integration, because it should never load...just like if you define "sensors:" without any entries the sensor block will not load.

JohnKiller commented 5 months ago

So a custom_componend shouldn't use the ModBus integration, but I have to reimplement everything it does in my component? Like this:

    modbushub = get_hub(hass, "HAIER-AC")
    coordinator = ModBusCoordinator(hass, modbushub)

Is there another way? How I'm supposed to do this?

Thanks again

JohnKiller commented 5 months ago

Also, the validation logic is still wrong, because the variable minimum_scan_interval is reset per device group instead of per hub. Please, take a look at my pull request, I think it's better to move the declaration outside.

janiversen commented 5 months ago

I have no idea if there is another way ! You are using the modbus integration in a way it was not intended to be used.

It is correct to reset it pr entity (NOT device) group, because we want to allow different values for different groups. Anyway it is a warning and not an error. Please note I make a review comment, which means I reviewed your PR, so no need to ask me to look ask at it again.

The error occurs later, due to entities NOT defined in the integration, which is not a bug in the modbus integration, but something that should be solved in the custom component.

JohnKiller commented 5 months ago

Sorry for asking to review the pr, I did not see your comment as I was replying here.

I do not want to insist on changing things, or to take any of your time. I just find very hard to try and create a good integration, because the usage of ModBus is very low and examples are hard to find.

Anyway, since ModBus is a communication protocol, I thought that configuring it in the main yaml was the correct thing to do, so that other integrations could use it. Instead, this integration tries to do everything by itself, and the moment that a device is not able to be configured with the options you have on the yaml, then you are out of luck.

Having the ModBus code duplicated in my integration is, in my opinion, bad, and also it doesn't allow to share the serial device with the main implementation.

Like, if I want to create an integration that uses BLE, I don't have to reconfigure the Bluetooth stack, I just use what's already there.

janiversen commented 5 months ago

There are a number of integrations that use the modbus integration, including core integrations, but they leave the configuration with modbus.

You can of course do as you please in a custom component, including duplicating the code, with the problems it carries when f.x. libraries are updated.