core icon indicating copy to clipboard operation
core copied to clipboard

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

Open il77781 opened this issue 1 year ago • 31 comments

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?

il77781 avatar Mar 21 '24 21:03 il77781

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)

home-assistant[bot] avatar Mar 21 '24 21:03 home-assistant[bot]

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 avatar Mar 22 '24 11:03 jbouwh

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 avatar Mar 22 '24 11:03 jbouwh

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

jbouwh avatar Mar 22 '24 12:03 jbouwh

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)

home-assistant[bot] avatar Mar 22 '24 12:03 home-assistant[bot]

@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?

il77781 avatar Mar 23 '24 08:03 il77781

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.

jbouwh avatar Mar 23 '24 09:03 jbouwh

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

il77781 avatar Mar 23 '24 09:03 il77781

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

Yes, sure

jbouwh avatar Mar 23 '24 09:03 jbouwh

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

il77781 avatar Mar 23 '24 09:03 il77781

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 avatar Mar 23 '24 09:03 jbouwh

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

jbouwh avatar Mar 23 '24 09:03 jbouwh

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

il77781 avatar Mar 23 '24 09:03 il77781

@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

jbouwh avatar Mar 23 '24 11:03 jbouwh

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

il77781 avatar Mar 23 '24 12:03 il77781

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.

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...

il77781 avatar Jun 22 '24 18:06 il77781

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

jbouwh avatar Jun 22 '24 20:06 jbouwh

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

il77781 avatar Jun 23 '24 17:06 il77781

@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.

jbouwh avatar Jun 28 '24 11:06 jbouwh

So what does it take to do that?

il77781 avatar Jun 28 '24 19:06 il77781

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.

jbouwh avatar Jun 28 '24 20:06 jbouwh

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?

il77781 avatar Jun 28 '24 20:06 il77781

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?

jbouwh avatar Jun 29 '24 07:06 jbouwh

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...

il77781 avatar Jun 29 '24 08:06 il77781

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.

jbouwh avatar Jun 29 '24 11:06 jbouwh

@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...

il77781 avatar Jun 29 '24 16:06 il77781

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.

Unfortunately, the problems I have described still exist... Apparently, there are no plans to change (improve) anything... In this regard, if necessary, you can close this issue...

il77781 avatar Sep 29 '24 20:09 il77781

The whole question here is, if this is an mqtt issue. Yes I know you experience this using an mqtt number box. But if you would create a number helper and enter the number out of the step range, you will notice it will update the entity. So to come to conclusions. The question is if it should be possible to enter other values, and where the validation should be done. Some run time validations are done in the core component. The general rule is that the device that receiving an invalid value (e.g. over mqtt) is reporting the actual value. It does not need to accept the invalid step value entered.

To prevent sensing an invalid value to the device you could use a command template to ensure the a valid value is send, e.g. by using a round function. At least this issue seems not going to be solved in it current form so I suggest to close it here.

jbouwh avatar Oct 05 '24 10:10 jbouwh