The "step" variable does not work for mqtt number in the box mode

il77781 commented 4 months ago

The problem

Hello! I found the following problem when configuring mqtt number in box mode: I specify the step variable and assign it a certain value... However, then, when I enter a value in the mqtt number cell that is not a multiple of the specified step value (for example, step=300, and I enter 450), a red stripe appears along the lower border of the cell, indicating that an incorrect value has been entered! However, after pressing the "enter" key (or exiting the cell by selecting any other interface element), the incorrect value entered into the cell is still sent in the mqtt message!!! In my opinion, this should not happen, but it should be like this: a message should appear similar to the one that appears when the entered value is less than the lower limit (variable min) or greater than the upper limit (variable max) - and at the same time the entered incorrect value should not be sent in the mqtt message... It seems to me that this needs to be corrected!

What version of Home Assistant Core has the issue?

core-2024.3.1

What was the last working version of Home Assistant Core?

This behavior is also observed in all previous versions

What type of installation are you running?

Home Assistant Supervised

Integration causing the issue

MQTT number

Link to integration documentation on our website

https://www.home-assistant.io/integrations/number.mqtt/

Diagnostics information

No response

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

Perhaps, again, this is not exactly the mqtt number problem, but the number (or input number) problem in general... But how to find out exactly?

home-assistant[bot] commented 4 months ago

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

Code owner commands

Code owners of `mqtt` 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 mqtt` 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)}}

mqtt documentation mqtt source _{^{(message by IssueLinks)}}

jbouwh commented 3 months ago

Right, it seems the frontend still calls the API. So to start with, this should not happen. May be we should filter these calls in the backend as well.

jbouwh commented 3 months ago

May be you can open a frontend issue for this? It is not an MQTT specific issue. In the detail dialog, the red border is not shown either.

About the core behavior:

The step attribute is not limiting the set of valid values. As floats are used, this could be tricky. The integration, in this case mqtt should solve this.
An mqtt number entity exposed by a device, should not allow invalid values. So when it posts back the state, this is what the actual state should be. In optimistic mode, this is not possible, as we cannot get back feedback from the device in optimistic mode.
There are no plans to add validation the the number.set_value service based on the step size, but you could use a command template to fail any invalid values.

jbouwh commented 3 months ago

May be there we need to add a validation in the number entity component.

home-assistant[bot] commented 3 months ago

Hey there @home-assistant/core, @shulyaka, mind taking a look at this issue as it has been labeled with an integration (number) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of `number` 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 number` 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)}}

number documentation number source _{^{(message by IssueLinks)}}

il77781 commented 3 months ago

@jbouwh, Thank you for your work! Do I understand correctly that in order for it to be used, it must first be "checked" by someone from a very limited circle of people?

jbouwh commented 3 months ago

The very limited circle is not too limited, but it might take a few days, and it must be conformant with the current architecture. It is still important to open the front-end issue to or open a PR with a fix.

il77781 commented 3 months ago

@jbouwh, as I understand, there is 10 people in this circle... ) Do I need to open the front-end issue now?

jbouwh commented 3 months ago

@jbouwh, as I understand, there is 10 people in this circle... ) Do I need to open the front-end issue now?

Yes, sure

il77781 commented 3 months ago

Do I need to indicate there that you have prepared a PR?

jbouwh commented 3 months ago

You can link it, but the frontend needs a front end fix too, as it submits a command while the validation says it should not be possible.

jbouwh commented 3 months ago

Note the front end is not a new issue here, but at the frontend repo

il77781 commented 3 months ago

@jbouwh, I'm quite new here... Please give me a link where and what needs to be done!

jbouwh commented 3 months ago

@jbouwh, I'm quite new here... Please give me a link where and what needs to be done!

https://github.com/home-assistant/frontend/issues

il77781 commented 3 months ago

@jbouwh, thank you! Done - https://github.com/home-assistant/frontend/issues/20168 I hope I did everything right?

issue-triage-workflows[bot] commented 4 weeks ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue has now been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.

il77781 commented 3 weeks ago

Unfortunately, the problems I have described still exist... I hope that competent people will still have the time and desire to fix this... In this regard, please do not close this issue and do not mark it as stale...

jbouwh commented 3 weeks ago

Merely I think this is a frontend issue, so I suggest we close it here for now.

il77781 commented 3 weeks ago

@jbouwh, that is, it will be necessary to return to this problem only after the problem with frontend is solved?

jbouwh commented 3 weeks ago

@jbouwh, that is, it will be necessary to return to this problem only after the problem with frontend is solved?

I am not sure about that. As the unit can change the step and scaling being factors or 10 this is hard to fix in the backend. First, to start with the frontend should just work as expected.

il77781 commented 3 weeks ago

So what does it take to do that?

jbouwh commented 3 weeks ago

I think this should be solved in the frontend. There is no hard step possible as conversion can disturb this. Integrations need to handle and thus validate the value. It sounds reasonable to handle the step handling in the frontend. A good example where the handling is challenging is when a temperature is set in Fahrenheit where the backend uses Celcius. In the front-end the question is then what the step size should be, and the offset, if the range starts with a fraction.

il77781 commented 3 weeks ago

That is, in principle, the only question is, at what stage is the verification of compliance of the entered value with the selected step performed? For example, a value in Fahrenheit is entered in the frontend - and at this stage it should be checked for compliance with the step... Right? And there shouldn't be any problems at this stage, right? And then, it turns out, in the backend, the converted value should no longer be checked for compliance with the converted step... Or did I completely misunderstand you?

jbouwh commented 3 weeks ago

The backend integrations should be responsible to round a value to a step, not the number integration should do this. What a step will be for a user, depends on the conversion. This part is not clear at all. So may be there should be a relation, but e.g. if Celcius is converted to Fahrenheit, do we still want to have the still fraction in the frontend if not the native UOM is used?

il77781 commented 3 weeks ago

Then maybe a simpler solution is to ban the conversion of units of measurement? And in the integration documentation, in this case, it should be clearly stated that the units of measurement in the backend and frontend should be strictly the same... Otherwise, the correctness of the work is not guaranteed...

jbouwh commented 2 weeks ago

Then maybe a simpler solution is to ban the conversion of units of measurement? And in the integration documentation, in this case, it should be clearly stated that the units of measurement in the backend and frontend should be strictly the same... Otherwise, the correctness of the work is not guaranteed...

I believe the user should not be bothered with the details of the backend, but be helped with a correct/fitting step size in the UI. There should not be an issue if the number device class is not set https://developers.home-assistant.io/docs/core/entity/number/. But when it is set the UI steps should be calculated to match with the original step unit. E,g, if the the amount of mA of current is set, and the step size is 10 mA, Then when A is set in the UI, the step size should be 0.01. In cases like the Farenheit to Celcius conversion, The (calculated) step size should be logical an about match the backend step size. The backend integration should round any values id that is needed. Logical means:

no fractions but factors that care logical. E.g. a step of 0.333 should become 0.5, 0.2 or 0.1. Also should the start of the range be with the calculated step factor.

This all is far from easy to implement and to support. Further an architectural discussion might be good to have in this case.

il77781 commented 2 weeks ago

@jbouwh, I completely agree with you that an ordinary user like me should not delve into various nuances... And, of course, he must receive a ready-made solution that provides for various situations. In addition, I agree with the approach you have proposed. But, unfortunately, at the moment, verification of the compliance of the entered value with the specified step is not performed at all, even in some limited form (since restrictions are also not implemented)... Discussions with competent developers are, without any doubt, very useful and important, but, unfortunately, for some reason no one is particularly eager to discuss the problem that has arisen... And that, of course, is frustrating...

home-assistant / core