openhab / org.openhab.ui.habmin

HABmin - a graphical user interface for openHAB 2
231 stars 92 forks source link

Changing scale on Temperature channels doesn't work #263

Closed mhilbush closed 6 years ago

mhilbush commented 7 years ago

Changing the scale on a Temperature channel doesn't work in HABmin. Doing the same thing in PaperUI works.

Behaves the same for all my battery devices with temp sensors (ZW100, ST814, and FGMS001). In HABmin, when I change the scale to Fahrenheit on the Temperature channel, it doesn't seem to work. HABmin still reports it as Celsius in the channel panel (after refreshing page), and I see in the log that the binding is not converting the reported temp to F.

Deleted node xml and deleted/readded things.

Looking at the Thing JSON for those nodes, they show this after changing the scale from C to F. Shouldn't config_scale be a 1 for F?

          "label": "Sensor (temperature)",
          "configuration": {
            "properties": {
              "config_scale": "0"
            }

When I do the edit using PaperUI, I see this in the thing JSON.

          "label": "Sensor (temperature)",
          "configuration": {
            "properties": {
              "config_scale": "1"
            }
          },
mhilbush commented 6 years ago

@cdjackson This still doesn't work in the most current 2.2 snapshot build. Setting the channel temperature scale doesn't work with HABmin but it does work with PaperUI.

mhilbush commented 6 years ago

Adding a little more info on this... Pressing save after changing the temp scale yields this rest PUT request, which appears to be missing the payload necessary for the change. Unfortunately, I'm having a hard time finding the spot in the code where this request is handled; otherwise, I'd suggest a fix. :-(

capture

mhilbush commented 6 years ago

Seems like you're detecting that config_scale has changed here: https://github.com/openhab/org.openhab.ui.habmin/blob/59b625429d0bce43b14055643d48125be51f19cb/src/web/app/configuration/thingConfig.js#L742

But that you're bailing out for some reason here: https://github.com/openhab/org.openhab.ui.habmin/blob/59b625429d0bce43b14055643d48125be51f19cb/src/web/app/configuration/thingConfig.js#L790

Not sure I understand what that last bit of code is trying to do...

Edit: I guess at L742 you're looking for changes to the Thing channel config. But at L790, you're looking only at the Thing config parameters. Therefore, even though you found a change to the channel config, you're not pushing the change because you found no changes to the Thing config parameters? At least that's what it looks like to me.

cdjackson commented 6 years ago

L790 isn’t relevant is it? This is just checking the thing configuration - it's not related to channel configuration by the looks of it. If you think this return statement causes the method to exit, it doesn’t - it’s inside a forEach loop, so it’s just exiting this loop. This last bit of code is performing conversions on the data to make sure it’s of the right type to be accepted on the server side (i.e. we don’t want to return “22” if the server wants an integer = 22).

The channel config is in the thing, so it’s setting thingUpdated=true. This is then saved at line 776, and at line 805 the system waits for all promises to be fulfilled.

I’d check that L743 is putting the data into the correct place.

I’ll try and look at this tomorrow or Monday - after that I’m on holiday for 2 weeks so time is a little short.

On 9 Dec 2017, at 16:34, Mark Hilbush notifications@github.com wrote:

Seems like you're detecting that config_scale has changed here: https://github.com/openhab/org.openhab.ui.habmin/blob/59b625429d0bce43b14055643d48125be51f19cb/src/web/app/configuration/thingConfig.js#L742 https://github.com/openhab/org.openhab.ui.habmin/blob/59b625429d0bce43b14055643d48125be51f19cb/src/web/app/configuration/thingConfig.js#L742 But that you're bailing out for some reason here: https://github.com/openhab/org.openhab.ui.habmin/blob/59b625429d0bce43b14055643d48125be51f19cb/src/web/app/configuration/thingConfig.js#L790 https://github.com/openhab/org.openhab.ui.habmin/blob/59b625429d0bce43b14055643d48125be51f19cb/src/web/app/configuration/thingConfig.js#L790 Not sure I understand what that last bit of code is trying to do...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openhab/org.openhab.ui.habmin/issues/263#issuecomment-350486449, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_kQ9scpCRS-ODzcH0VCfzLbB1xCl7Cks5s-raJgaJpZM4M-YRy.

mhilbush commented 6 years ago

Yes, you're right.

L748 is where you mark that the thing was updated due to a change in the channel config, but it doesn't look like that change is saved in updatedThing, so the change is never included in the rest call to update the thing.

I'll keep debugging...

mhilbush commented 6 years ago

I see, on L743 you're updating channel.configuration[model.$name] with the new config_scale value, but channel never gets added to updatedThing, therefore it's not pushed on L776. Problem is I don't know the correct syntax to add channel to updatedThing, so I can't try out a fix...

mhilbush commented 6 years ago

It looks to me like it broke with this commit, which doesn't account for modified channel config parameters.. https://github.com/openhab/org.openhab.ui.habmin/commit/ab4fb162eabafa3316fd166354504ff557d2135c#diff-f3bef2d013adf4add6c146920c06bc1a

At line 763, if I replace these two lines

updatedThing.UID = $scope.selectedThing.UID;
promises.push(ThingModel.putThing(updatedThing));

with the old code prior to the above commit, it works.

promises.push(ThingModel.putThing($scope.selectedThing));
cdjackson commented 6 years ago

Thanks. I’ll try and look at this tomorrow and see if I can reverse it.

On 9 Dec 2017, at 18:51, Mark Hilbush notifications@github.com wrote:

It looks to me like it broke with this commit, which doesn't account for modified channel config parameters.. ab4fb16#diff-f3bef2d013adf4add6c146920c06bc1a https://github.com/openhab/org.openhab.ui.habmin/commit/ab4fb162eabafa3316fd166354504ff557d2135c#diff-f3bef2d013adf4add6c146920c06bc1a At line 763, if I replace these two lines

updatedThing.UID = $scope.selectedThing.UID; promises.push(ThingModel.putThing(updatedThing)); with the old code prior to the above commit, it works.

promises.push(ThingModel.putThing($scope.selectedThing)); — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openhab/org.openhab.ui.habmin/issues/263#issuecomment-350497665, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_kQ_-l_lZRScxaMI3eBImJh3YR3pGgks5s-tazgaJpZM4M-YRy.

mhilbush commented 6 years ago

Fixed in habmin 2.3.0.201801131314