gpbenton / engMQTTClient

MQTT client for Energenie ener314-rt board running on a raspberry Pi
MIT License
21 stars 10 forks source link

Possible to have floating point target and target received temps? #30

Closed DrHoneyBear closed 3 years ago

DrHoneyBear commented 3 years ago

Examining the logs, it appears that target temps are being sent as XXXX:YY:tt

XXXX=TRV id as a Hex value YY = command (set temp) as a Hex value tt= temp as an integer decimal

The TRV target temp (Target Temperature Received) is an integer, and in HomeAssistant the reciprocal return value for target temp is also handled and displayed as an integer.

However, current temperate returned by the TRV and displayed in HomeAssistant is a float to 4 decimal places.

It would be desirable to have all in floating point although 4 decimals seems an overkill but inconsequential to setups that have the typical thermostats with 1 decimal step values, such as 19.0, 19,5, 20, 20.5 and so on.

gpbenton commented 3 years ago

I am afraid https://github.com/gpbenton/engMQTTClient/issues/15#issuecomment-497317544 still applies.

I'm happy to accept a PR if someone wants to add support for decimal places, but I don't need it.

This project also suffers from the problem that the only board I have is used on the long dark nights of winter, which is when I am likely to be interested in sitting down and thinking about it :-(

DrHoneyBear commented 3 years ago

Fair enough you do not need 0.5 step intervals, but given you have shared this solution to the community, there's value to be had in facilitating at least 0.5 steps , half a degree is quite effectual - relatively.

I will happily attempt to improve your code but need your help please.

First, I note in engMQTTClient.c in function my_message_callback around** line 449, the line:

uint32_t temperature = strtoul(tempString, NULL, 0);

So you are converting a string (for the desired temp) such as "20.5" to an unsigned long integer. I presume long = 32 bit based on your later code where a 0xff mask is applied - I will touch on below? This str to long would need to change to allow set points such as 20.5 (Celsius).

Further, I note the following code in engMQTTClient.c that appears to force 16 bit unsigned int as the payload, ...

             case OT_TEMP_SET:
                            log4c_category_debug(clientlog, "Sending Set Temperature %d to device %d",
                                                 commandToSend->data, msgData.sensorId);
                            HRF_send_FSK_msg(HRF_make_FSK_msg(engManufacturerId, encryptId, 
                                                              eTRVProductId, msgData.sensorId,
                                                              4, OT_TEMP_SET, 0x92, 
                                                              (commandToSend->data & 0xff), 
                                                              0x00), 
                                             encryptId);

                            {
                                // Report temperature set to MQTT broker
                                char mqttTempSetTopic[strlen(MQTT_TOPIC_SENT_TARGET_TEMP) 
                                    + MQTT_TOPIC_MAX_SENSOR_LENGTH 
                                    + 5 + 1];

                                snprintf(mqttTempSetTopic, sizeof(mqttTempSetTopic), "%s/%d", 
                                         MQTT_TOPIC_SENT_TARGET_TEMP, msgData.sensorId);

                                // Should only be 1 or 2 digits for temperature
                                char temperature[5];
                                snprintf(temperature, 4, "%d", commandToSend->data);

                                mosquitto_publish(mosq, NULL, mqttTempSetTopic, 
                                                  strlen(temperature), temperature,
                                                  0, false);
                            }
                            break;

in the above, the critical call is: HRF_send_FSK_msg(HRF_make_FSK_msg(engManufacturerId, encryptId, eTRVProductId, msgData.sensorId, 4, OT_TEMP_SET, 0x92, (commandToSend->data & 0xff), 0x00), encryptId);

where (commandToSend->data & 0xff), is masking only the 16 bit of the 32bit long integer. Again, as in my opening comment above, the concept of dealing with unsigned integers precludes any setting of fractional temps such as 20.5 C.

So this all begs the question, as to why you chose to pass a 16bit unsigned int to the HRF FSK interface. Is there something in the interface from Energenie that prevents use of fractional temps in payloads sent to the sensor, but allows fractional temps to be received. I state the latter because your code does actually report fractional temps from the State topic "Temperature" which is good. Are you able to provide any guidance on how best to effect the desired change to allow fractional temps to be sent to the sensor - please.

gpbenton commented 3 years ago

From what I remember, and its a while since I looked at this so I may be wrong, but the second byte reflects the fractional part of the value, and the msb of that byte represented 0.5

If all you need is .5 degrees accuracy, if the first part of the fraction in the command is greater than 5, set the msb of the second byte.

But as I say, it has been a few years, so I may be remembering it all wrong.

DrHoneyBear commented 3 years ago

thanks for swift reply Graham, but to clarify:-

DrHoneyBear commented 3 years ago

bump after 11 days ?

gpbenton commented 3 years ago

I'm sorry, I have said all I can remember. The rest you will have to work out for yourself.

DrHoneyBear commented 3 years ago

Well, couldnt work it out. Not many comments to go with and same for much of the source which you lifted from the MiHome codebase. I can see that there seems to be some limitation on the tx of the value, by means of a cast to int and then some bitwise manipulation. Too time consuming so working on a Home Assistant Supervised (formerly known as hassio) addon for you and will upload this to the HA community addons site. This will make it uber simple for anyone to add your mqtt client/broker to a running HA Supervised installation.

gpbenton commented 3 years ago

I am not sure I like the term 'lifted'. It implies some kind of nefarious back door method. The example software I used is freely available on the energenie web site here

Please make sure that it is clear that whatever you produce is your work, and not directly related to this project. I do not want to be bothered by questions about HA addons.