Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements to the API #46

Closed
proddy opened this issue Apr 19, 2021 · 42 comments
Closed

Improvements to the API #46

proddy opened this issue Apr 19, 2021 · 42 comments
Labels
enhancement New feature or request

Comments

@proddy
Copy link
Contributor

proddy commented Apr 19, 2021

As reported from @tp1de

While using rest api the return values from info command per device returns json on data fields with description and not with names of data fields.
Is there any reason why not returning the field names? By the way when using the id return json is different and contains datafield names:
eg mixer http://ems-esp/api?device=mixer&cmd=info vs http://ems-esp/api?device=mixer&id=2&cmd=info:
{"(hc2) setpoint flow temperature":27,"(hc2) flow temperature in assigned hc (TC1)":29,"(hc2) pump status in assigned hc (PC1)":"on","(hc2) mixing valve actuator in assigned hc (VC1)":5}
{"id":160,"flowsettemp":27,"flowtemphc":29,"pumpstatus":1,"valvestatus":5}

Two things to do here:

  • fix the formatting if an id or hc is provided in the URL
  • decide whether to use verbose or short names
@proddy proddy added the bug Something isn't working label Apr 19, 2021
@tp1de
Copy link
Contributor

tp1de commented Apr 19, 2021

For integration into other home automation systems short names (datafield names) would be needed for rest API json.
(unless mqtt broker is used for integration)

@proddy
Copy link
Contributor Author

proddy commented Apr 19, 2021

either we make it all use shortnames (my preference) or have an configurable option short/verbose in the WebUI or make an additional command for verbose naming like info_verbose

@MichaelDvP ?

@MichaelDvP
Copy link
Contributor

MichaelDvP commented Apr 19, 2021

There was a discussion a while ago, can't remember the number. As result:

v2 only uses short names, the long ones are not available for API.
id=0 (and default) outputs complete nested structure, id=1..4 only one hc unnested.

v3 defaults to long names (id=-1), with id=0,1..4 output is compatible with v2.

Edit: Discussion was in #698, I've checked the output on my system and works as expected, can't see any formatting issue or bug.

@tp1de
Copy link
Contributor

tp1de commented Apr 19, 2021

@MichaelDvP @proddy
I can confirm that id = 0 works but for me it was a "hidden secret" and is not mentioned in the API documentation.
I was just used to id within thermostat and mixer (hc) and it was a big surprise that I need to use id=0 for boiler as well to get short names with info command.

It should be better within documentation .... (if even @proddy does not remember anymore :) and not just new for me...)

@MichaelDvP
Copy link
Contributor

for me it was a "hidden secret" and is not mentioned in the API documentation.

Yes, the doc is based on v2.2.1 as mentioned here, and most people using v2, there is not an extra doc for v3.
@proddy What do you think? Tabs on every page that is different in v3, or switch the doc to v3, or an extra doc?

@tp1de
Copy link
Contributor

tp1de commented Apr 19, 2021

I would recommend to avoid unnecessary API / MQTT changes between v2 and v3.

So why not keeping short names by default in v3 for cmd=info and adding a new command cmd=verbose for full names?
Then documentation is still valid and just not covering new functionality. This can be added in new tab or extra text.

When trying to build interfaces the respective number values are important and not so much the corresponding expressions.
The API documentation actually is not covering all of these values as well - e.g. for modes / building parameters etc....

I would highly appreciate If a new command per device or datafield could be introduced: cmd=config which returns:
writable yes/no, min and max values allowed, allowed states similar to hhtp get commands for km200.
E.g.:
{"id":"thermostat.hc1.mode","type":"stringValue","writeable":1,"allowedValues":["0:"auto","1":"manual"]}
{"id":"thermostat.hc1.summertemp","type":"floatValue","writeable":1,"unitOfMeasure":"°C","minValue":10,"maxValue":30}

@proddy
Copy link
Contributor Author

proddy commented Apr 20, 2021

It should be better within documentation .... (if even @proddy does not remember anymore :) and not just new for me...)

That's true! I'm so occupied with work these days I'm started to forget what we coded in EMS-ESP. Luckily Michael seems to have a memory like an elephant.

Creating a new API command that sends back a JSON array with the id, shortname, type, writeable is possible. If that really helps.

And @MichaelDvP adding a min & max value or an enum would also be handy, especially in the new Web UI where values can set and also used in Home Assistant (and perhaps Domoticz).

@MichaelDvP
Copy link
Contributor

And @MichaelDvP adding a min & max value or an enum would also be handy, especially in the new Web UI where values can set and also used in Home Assistant (and perhaps Domoticz).

Only problem is, that we do not know the min/max and they differ for different boiler/thermostat types. If someone provides this data for all types it can be done. But it's a lot of work with only little value even if we have a table with all data.

@proddy
Copy link
Contributor Author

proddy commented Apr 21, 2021

good point. it's a lot of guess work and there are bound to be many variations across our 80 supported devices.

@tp1de
Copy link
Contributor

tp1de commented Apr 21, 2021

I support your argument that it is difficult to get the configurations for 80 supported devices. Nevertheless there are a couple of critical parameters valid for all devices where a min/max value control should be implemented at least within ems-esp firmware to secure stable operations of heating system and avoiding wrong end-user input. Direct changes by ems-bus are anyhow critical, so I would strongly recommend that some basic testing routines on valid ranges should be implemented for the most critical datapoints. These are only a few:

  • boiler hysteresis - only positive values for boilhystoff and negative values for boilhyston - what should be a minimum hysteresis?
  • burnminperiod : values between 5 and 60
  • wwsettemp: values between 10 and 75 - above 60 gives a warning for boiling risks on tapwater.
  • wwdisinfectiontemp: between 60 and 75
  • and all fields with range of state values - e.g. wwcircmode for boiler with range 0-7

While writing an interface to home automation systems I would like to get your feedback on how to implement this input control?
In my case (ioBroker) I use a csv file which defines each field (ems-esp and km200) where unit of measure, min/max ,valid states and writable y/n is defined. For km200 datafields I get this info by http get request per field and for ems-esp I maintained it manually (it took quite some time to find out). As a result ioBroker states can only be changed within the defined value-ranges and if they are writable. This logic works fine, but whenever ems-esp fieldnames change or new field are defined I need to adjust the csv-file.

Since I use MQTT as an interface to the respective ems-esp ioBroker-adapter I programmed, any commands for changes I send by mqtt are not replied and therefore can not be checked if an error happened. E.g. trying to change a field which is not writable.
Shall I use the rest API and check the resonse? What would be your recommendation for integration of other systems?

@proddy
Copy link
Contributor Author

proddy commented Apr 21, 2021

Yes, I would only use the rest API for doing write operations. If the command is not writable it'll come back with an error like "Invalid cmd". When I finally get to moving the web server and adding SSL the API will change and be more according to the proper API standards (like https://stackoverflow.blog/2020/03/02/best-practices-for-rest-api-design/) and make it easier for you.

I do agree adding some validation to the write settings would be useful. I've had to do a number of factory resets on my boiler when testing and setting bogus values by mistake. Need to think about that one. It's a lot of work as Michael said!

@proddy proddy added enhancement New feature or request and removed bug Something isn't working labels Apr 21, 2021
@proddy proddy changed the title Inconsistency in API info command Improvements to the API Apr 21, 2021
@proddy
Copy link
Contributor Author

proddy commented Apr 21, 2021

I've renamed this issue, as it was originally about the info command and hidden feature to get short-names. Now it's about general improvements to the API and an additional command to bring back the capabilities.

@MichaelDvP
Copy link
Contributor

valid ranges should be implemented for the most critical datapoints. These are only a few:

And here is the problem i mentioned, half of the ranges you describe is not right for my boiler. hyston can be positive, usefull to have an offset, wwsettemp is limited to min:30 for me, disinfection can be 80, and so on. And i think this is also for much more values (like modulation-ranges specific for pump-types or burners). Also enum-values differ much, e.g. building light/medium/heavy is 0,1,2 for RC35, but 1,2,3 for RC300, some thermostats uses modesettings off/on/auto, some off/auto/on, and so on.

any commands for changes I send by mqtt are not replied and therefore can not be checked

All commands makes a postvalidation with a bus-read and publishes the result out of order via mqtt.

We have also the publish of topic response which gives information back for some commands (like raw reads). We can use this for feedback or for publishing the range.
For instance: send command with value response:{ok|failed}, send command without value: response: {"type":"number","min":0, "max:90, "unit":"°C"}
But for ok/failed we can only check if the command is send to the device, not if the device has accepted the value. To check the value change in the device it needs a lot of code-changes.

To check for writeable commands v3 has the option for direct subscribes, which will show a handy list of all commands.
instead of boiler={"cmd":"wwsettemp","data":50} you can check subscription of boiler/wwsettemp and send boiler/wwsettemp=50.

@MichaelDvP
Copy link
Contributor

@tp1de I've pushed a version with info for each value. Working on API, mqtt and console. Output is a json like

ems-esp2:# call thermostat building
{
  "name": "building",
  "value": 1,
  "type": "enum",
  "min": 0,
  "max": 2,
  "writeable": true
}
ems-esp2:# call boiler wwseltemp
{
  "name": "wwseltemp",
  "value": 48,
  "type": "number",
  "min": 0,
  "max": 255,
  "unit": "°C",
  "writeable": false
}

Min/max is set in enum for possible values, all number values gives the range of storage (int/short/long), value is only set if there is a valid value.

@tp1de
Copy link
Contributor

tp1de commented Apr 22, 2021

@MichaelDvP

this is super. I will wait for further changes on the ioBroker Adapter until you are finished.
I understood that that I rather should use rest API for polling / sending to get immediate response.
The MQTT response is time-delayed and it is tricky to process in the coding of the adapter.
Is this the right understanding?

Also enum-values differ much, e.g. building light/medium/heavy is 0,1,2 for RC35, but 1,2,3 for RC300, some thermostats uses modesettings off/on/auto, some off/auto/on, and so on.

Are you intending to include the enum-values and as well or can't this be done?

@tp1de
Copy link
Contributor

tp1de commented Apr 22, 2021

@MichaelDvP

I tested the new version. Units and writable info seems to work fine
I just wonder about min/max values.

ems-esp:# call thermostat designtemp
{
  "name": "designtemp",
  "value": 80,
  "type": "number",
  "min": 0,
  "max": 255,
  "unit": "°C",
  "writeable": true
}
ems-esp:# call thermostat controlmode
{
  "name": "controlmode",
  "value": 1,
  "type": "enum",
  "min": 0,
  "max": 6,
  "writeable": true
}

e.g. controlmode can be 1 or 2 (outside / inside)

@MichaelDvP
Copy link
Contributor

The MQTT response is time-delayed and it is tricky to process in the coding of the adapter.

The delay in publish device_data after sending a value is from ems-bus, We send the new value, read it back from bus and publish immediatly the mqtt. Only readback from different telgrams is delayed, because the device needs some time to set it. Mqtt-response-feedback for the value-info is immediatly, it triggers no bus bus-activity and do not have to wait.

Are you intending to include the enum-values and as well or can't this be done?

Do you mean the names? How? As json-array "enum":["light","medium","heavy"]? You can always send the enums/bools as numbers or string.

e.g. controlmode can be 1 or 2 (outside / inside)

For RC300 it is off, outdoor, simple, mpc, room, power, constant, but don't ask me what mpc means, this is listed in Norberts document.

@tp1de
Copy link
Contributor

tp1de commented Apr 22, 2021

Are you intending to include the enum-values and as well or can't this be done?

Do you mean the names? How? As json-array "enum":["light","medium","heavy"]? You can always send the enums/bools as numbers or string.

The km200 reply from http get returns for allowed values (enums) an array for allowedValues with number and strings: array[3]
[0: "light",1: "medium",2: "heavy"] that is what I would recommend.

I have seen that in the dashboard values are now changeable as well. For enum you need to know what to enter. A drill-down menu would be easier.

For RC300 it is off, outdoor, simple, mpc, room, power, constant, but don't ask me what mpc means, this is listed in Norberts document.

I just checked on my RC310. I was wrong too. I have 3 choices: outdoor / outdoor with footpoint / indoor.
I agree that it will be difficult to define all variants of enums for different device combinations.
But there might be 3-4 "standard" configurations - like mine with rc310 and 2 hc's (radiators and floorheating).

I do not know if it is worthwhile maintaining key variants or if it is possible to make the enum values configurable in ems-esp.
I will make the "allowedValues" changeable in the ioBroker-adapter for ems-esp datapoints / states.
The other states from km200 interface is giving already the right information.

@MichaelDvP
Copy link
Contributor

Do you mean an array with [, this contains only a list, or a nested json with {
I can do:

ems-esp2:# call thermostat building
{
  "name": "building",
  "value": "medium",
  "type": "enum",
  "min": 0,
  "max": 2,
  "enum": [
    "light",
    "medium",
    "heavy"
  ],
  "writeable": true
}

I just checked on my RC310. I was wrong too. I have 3 choices: outdoor / outdoor with footpoint / indoor.

I can't find this setting in Norberts list. You have to log a change to find the telegram and place.

@tp1de
Copy link
Contributor

tp1de commented Apr 22, 2021

Do you mean an array with [, this contains only a list, or a nested json with {

yes a nested json with array like you described (similar to km200 get return json) .
In km200-json enum is just named allowedValues and type is stringValue instead of enum.

Your proposal is excellent!

I can't find this setting in Norberts list. You have to log a change to find the telegram and place.

There are a couple of other changes and missing datafields compared to km200.
Let's do the API changes first. I will make a proposal of datafields which can be added or need some adjustment afterwards.

@tp1de
Copy link
Contributor

tp1de commented Apr 23, 2021

@MichaelDvP
I have seen that you already implemented the nested json with enum to the console command. 👍
Just tell when this command is available by rest API and how to access (by field / by device)

@tp1de
Copy link
Contributor

tp1de commented Apr 23, 2021

@MichaelDvP
Just a small observation / remark;
The api command http://ems-esp/api?device=system&cmd=info&id=0 returns device informations:

.......
Devices": [
    {
      "type": "Boiler",
      "name": "Buderus GB125/Logamatic MC110 (DeviceID:0x08 ProductID:133, Version:01.44)",
      "handlers": "0x10 0x11 0x14 0x15 0x1C 0x18 0x19 0x35 0x16 0x33 0x34 0x1A 0x26 0x2A 0xD1 0xE3 0xE4 0xE5 0xE6 0xE9 0xEA 0x494 0x495"
    }, 
......

By trying to poll on devices within this nested json and taking type for polling with http://ems-esp/api?device=Boiler&cmd=info&id=0 returns an error due to the capital letter in type.

Maybe it is worthwhile considering to add an entry device which can be used for polling and/or allowing capital letters within device-names.

@MichaelDvP
Copy link
Contributor

As mentioned it is already implemented for console, API and mqtt.
console: call thermostat building
API: http://ems-esp.local/API?device=thermostat&cmd=building
mqtt: thermostat = {"cmd":"building"}
simple rule: if data is empty you get the value-info back.

A few values with command only (temp, wwtapactivated, reset) are missing for now. Coming soon.

I'll also want to implement id/hc to get the value from a specific heating circuit, Since the console command can not have id without data, i'll use a wildcard ? as data, call thermostat daytemp ? 2 will give the value from hc2, for api/mqtt this is optional: api?device=thermostat&cmd=daytemp&id=2 or api?device=thermostat&cmd=daytemp&data=?&id=2 is the same result. Without id you get the value from first active hc.
But this needs some testing and fixing before i'll push it..

@MichaelDvP
Copy link
Contributor

Maybe it is worthwhile considering to add an entry device which can be used for polling and/or allowing capital letters within device-names.

Ok, i'll convert the input to lowercase before comparing.

@tp1de
Copy link
Contributor

tp1de commented Apr 23, 2021

As mentioned it is already implemented for console, API and mqtt.

Yes you are right - I haven't understood the command.
Just id parameter is not working yet as you explained.
A question about units: Whenever unit is °C I get °C back. How can this be avoided?
{
"name": "designtemp",
"value": 80,
"type": "number",
"min": 0,
"max": 255,
"unit": "°C",
"writeable": true
}

@tp1de
Copy link
Contributor

tp1de commented Apr 23, 2021

.... and should the name not include the heating circuit? e.g. hc1.designtemp / hc2.designtemp

@MichaelDvP
Copy link
Contributor

Whenever unit is °C I get °C back. How can this be avoided?

Switch your browser view/textcoding from uincode to auto or utf8. Degree is not ascii and depends on charset.

@tp1de
Copy link
Contributor

tp1de commented Apr 23, 2021

Switch your browser view/textcoding from uincode to auto or utf8. Degree is not ascii and depends on charset.

I haven't found a way in the browser (firefox / chrome or edge) if I enter directly in the adressfield. Shouldn't utf8 not beeing defined from ems-esp response?

Nevertheless if I send a http get request from javascript with utf8 encoding I get in return the right characters ....

@MichaelDvP
Copy link
Contributor

Shouldn't utf8 not beeing defined from ems-esp response?

Yes, set request->send(200, "text/plain;charset=utf-8", buffer.c_str()); to line 85 of WebAPIService.cpp.

@MichaelDvP
Copy link
Contributor

I've pushed the id/prefix, device-lowercase, and utf-8 changes. Also made a test with min/max stored for each value, but it's a lot of change and extra memory and @proddy have to check if it fit's in his plans for ems-esp.

@tp1de
Copy link
Contributor

tp1de commented Apr 24, 2021

@MichaelDvP
Thanks Michael, this a big step forward and it helps a lot in building interfaces. I will test as well.

By the way: min/max values are only needed for writeable fields.
The enum's and units are much more important and it might be helpfull to store enum definitions per device. (limited number)

@tp1de
Copy link
Contributor

tp1de commented Apr 24, 2021

@MichaelDvP
Everything is fine, id available (circuit) and °C now displayed correctly as well.
I just would recommend to send the enum entry for boolean values as well (off,on).

I want to seperate boiler (heating) and ww fields like beeing done by mqtt topic by reading fields now with rest API. Can this be done by reading the first two letters from fieldname "ww" or is there any other identifier available?

@tp1de
Copy link
Contributor

tp1de commented Apr 25, 2021

@MichaelDvP
I just looked at the latest version (b4) from today and have seen the changes on boolean. E.g. for boiler:
{"name": "wwcircpump","value": true,"type": "boolean","min": 0,"max": 1,"enum": [false,true],"writeable": true}
The enum should contain the values you display in the dashboard:[on,off] - isn't it?

@tp1de
Copy link
Contributor

tp1de commented Apr 25, 2021

The device boiler contains one fieldwwHeatwhere the http request http://ems-esp/api?device=boiler&cmd=wwheat returns "Invalid". Uppercase wwHeat doesn't work as well.

@tp1de
Copy link
Contributor

tp1de commented Apr 25, 2021

Since I am implementimg the ioBroker adapter based on rest api rather then mqtt, I recognized that the mqtt heartbeat and the system device info by rest api are different.
I would recommend to use the same fieldnames in the "Status" json as in heartbeat.(using id=0).
In rest API the following info is missing: freemem,uptime and rssi.

@proddy
Copy link
Contributor Author

proddy commented Apr 25, 2021

Since I am implementimg the ioBroker adapter based on rest api rather then mqtt, I recognized that the mqtt heartbeat and the system device info by rest api are different.
I would recommend to use the same fieldnames in the "Status" json as in heartbeat.(using id=0).
In rest API the following info is missing: freemem,uptime and rssi.

  • no problem moving uptime to Status in the API and adding freemem and rssi.
  • the wwheat is a typo in locale_EN.h

both really minor changes, I can do it or Michael if you're still busy with the code?

@MichaelDvP
Copy link
Contributor

I've fixed the wwHeat, but see the others to late. Create a API-System-info-id0 with shortnames is a bit more to do. I can do it tomorrow.

@proddy
Copy link
Contributor Author

proddy commented Apr 25, 2021

thanks Michael!

@MichaelDvP
Copy link
Contributor

I thought about it and found a simple solution that ensures that output is the same.

@tp1de
Copy link
Contributor

tp1de commented Apr 25, 2021

I thought about it and found a simple solution that ensures that output is the same

Sorry Michael a bit too fast ..... now the "System" and "Devices" part of the nested json disappeared.
Please only replace the "Status" json-substructure with the info of mqtt heartbeat .....

I need to read the "Devices" for selecting them for the rest-api device selector within the info-commands.

@tp1de
Copy link
Contributor

tp1de commented Apr 25, 2021

Sorry Michael a bit too fast ..... now the "System" and "Devices" part of the nested json disappeared.
Please only replace the "Status" json-substructure with the info of mqtt heartbeat .....

I need to read the "Devices" for selecting them for the rest-api device selector within the info-commands.

@MichaelDvP I changed my adapter to select one system part without id and Status part by using id=0
For me it is ok to keep this version. I just recommend to change the enum values for boolean to [off,on]
My ioBroker-Adapter is actually working fine and gets now all relevant ems-esp field parameters as well as from km200.

Thanks a lot for your support @MichaelDvP and @proddy

P.S. Next steps would be to compare the enums (allowedStates) - I see some differences between ems-esp and km200
and identify some field-errors related to my installation (RC310 / KB192i / MM100)

@proddy
Copy link
Contributor Author

proddy commented Apr 29, 2021

We can close this now. Thanks Michael for all the great work you put into this. I have a plan to improve the API (#50) so interested in your thoughts and whether this will be easier to integrate into iobroker @tp1de ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants