eq3device/eq3interface: #12

btsimonh · 2018-11-03T15:57:37Z

Hi Alikh31,

I've been working a lot with these devices now, and returned to noble (trying to make it work better!).

One of the things I've done is some heavy updates on the two lib files; in theory these mods don't change anything related to the node-red node, but I've NOT tested with the actual node. (all testing in function nodes so far).
If you are able to test in your setup, that would be great.

Details:
Make ALL sends parse the return.
Enhance return parsing.
add functions for schedules.
fix getInfo to setDate (it was screwing with my date/time on devices? - using the last time sent to it!).
fix various '0' fill left issues.
Add CC-RT-M-BLE.
return {error:} on error rather than nothing.
enhanced ecoMode ('holiday mode') setting with temp & date options.

I AM successfully using multiple devices with noble now.
So my next task is to review the node functionality with regard to what I want to achieve, and see if I can enhance it without disturbing existing users :). If I can, I'll provide another pull request.....

…rn parsing. add functions for schedules. fix getInfo to setDate, fix various '0' fill left issues. Add CC-RT-M-BLE. return {error:<error>} on error rather than nothing. enhanced ecoMode ('holiday mode') setting with temp & date options

btsimonh · 2018-11-03T16:03:50Z

just did a VERY quick test, and it the NR node seems to work :)

btsimonh · 2018-11-03T16:23:15Z

I notice in the node that you are setting a global[config.eq3device] = device.
Do you use this externally (e.g. in a function node), or do you know if anyone else uses it?.
One of the things I see as a need is to connect only when needed; else I don't think the 'phone app can work with the TRVs. Also connection management is quite evil in noble. I'm thinking about a similar approach of using a global, but to track all devices we've seen. Another thing I do is serialise all commands to each device. Also, some aspects of BLE are machine wide (e.g. scanning), and need to be dealt with appropriately. Other BLE related nodes will interfere without a little thought going in.

sorry, for the essay, but just thinking about how to approach the next step.
Another thing I'd like to consider is if the EQ3 devices are access over a BLE to MQTT gateway (if I can successfully build one! - built but not yet blown to my ESP32....)

alikh31 · 2018-11-03T16:25:30Z

@btsimonh thanks for your contribution. unfortunately I have no eq3 device in hand at the moment. if you are sure the code will fully function I am more than happy to merge it. would be great to test it further and make sure everything is in place.

If there are any changes to the api please update the readme file as well.

btsimonh · 2018-11-03T16:26:18Z

ok, will do more testing. thankyou...

alikh31 · 2018-11-08T16:50:37Z

@btsimonh is this ready to merge?

btsimonh · 2018-11-08T16:56:05Z

hi @alikh31,
give me a couple more weeks. It's pretty much there for what it does, but I'm still working hard on BLE in general, and that may result in some more re-work.
s

thomasnordquist

Really nice work!
It's really helpful that you added some documentation.

The comments are mostly small things and style related, I didn't find anything actually wrong =)

It's always a good idea to keep functions/methods small, and their scope to a specific task.

I could test your work so far if you would like, I mostly use the manual and boost functions and valvePosition, targetTemperature properties. And since I get new hardware this week, I would be able to test the pairing as well.
I have a bunch of these

thomasnordquist · 2018-12-03T22:29:38Z

lib/eq3interface.js

+              break;
+
+          case 2:
+              switch(info[1] & 0xf){


Each case block could simply invoke it's own function so you can get smaller functions with nicer names. Having a nice name you could even remove comments like // normal info.

case 1: return this->parsedSysinfo(); break; case 2: return $this->parsedNormalInfo(); break;

This becomes even more relevant when you have things like nested switch-case blocks (like a few lines below).

thomasnordquist · 2018-12-03T22:34:42Z

lib/eq3interface.js

+              };
+              break;
+
+          case 0x21:


It's unclear what the meaning of 0x21 is. A comment would be nice here.

thomasnordquist · 2018-12-03T22:38:57Z

lib/eq3interface.js

-      return new Buffer(`11${temp}${dur}`, 'hex')
+      const temp = ('0'+(2 * temperature).toString(16)).slice(-2);
+      const dur = ('0'+(minDuration / 5).toString(16)).slice(-2);
+      return new Buffer(`14${temp}${dur}`, 'hex')


Is this change intentional? (the change from 11 to 14)

yes :). seem to remember it was wrong...., some time agao now though.

thomasnordquist · 2018-12-03T22:50:59Z

lib/eq3interface.js

-      const tempNight = (2 * night).toString(16)
-      const tempDay = (2 * day).toString(16)
+      const tempNight = ('0'+(2 * night).toString(16)).slice(-2);
+      const tempDay = ('0'+(2 * day).toString(16)).slice(-2);


If I'm understanding '0'+( ... ).slice(-2) correctly, this is supposed to be a "left-pad" function.

While it is a neat way to expand a number to two digits, it is not very readable.
It would be nicer to have a function for it, with a name describing what happens:

function expandNumberToTwoDigits(str) { return ('0'+str).slice(-2) }

But seeing that what we actually want to do ist: convert an integer < 256 (and >= 0) to a two-digit hex value.

function byteToHex(number) { ('0'+(number).toString(16)).slice(-2); }

The call site would then look something like this:

const tempDay = byteToHex(2 * day);

thomasnordquist · 2018-12-03T22:53:19Z

lib/eq3interface.js

@@ -16,55 +16,251 @@ module.exports = {
  notificationCharacteristic: 'd0e8434dcd290996af416c90f4e0eb2a',
  serviceUuid: '3e135142654f9090134aa6ff5bb77046',
  payload: {
-    getInfo: () => new Buffer('03', 'hex'),
+    getSysInfo: () => new Buffer('00', 'hex'),


What is the reason for the changing the payload / buffer value?
Won't this break compatibility with other devices?

:). i remember now. I wondered why all my valves dates were screwed. 03 is setdate, and if any command is sent without the correct payload, the device seems to use either the last payload for that command, or maybe even just the remainign bytes of the last payload! So using 03 was setting the date back to the last date set by the phone app..... 00 seems to be the right command to send without a payload to get the status.

yes, so to get the status response, you must set the time/date. See earlier mods in eq3Device to change the getInfo call on the device level to call setDatetime. getSysInfo is a NEW command which gets a serial number, etc. (unknown format).

thomasnordquist · 2018-12-03T23:01:22Z

lib/eq3interface.js

+              // start firmware update
+              return {
+                firwareupdate:true,
+                raw:info,


mixed indentation

thomasnordquist · 2018-12-03T23:01:36Z

lib/eq3interface.js

+                      break;
+              }
+              return {
+                firwareupdate:true,


mixed indentation

thomasnordquist · 2018-12-03T23:02:52Z

lib/eq3interface.js

-      const minute = date.getMinutes().toString(16)
-      const second = date.getSeconds().toString(16)
-      return new Buffer(prefix + year + month + day + hour + minute + second, 'hex')
+      var out = new Buffer(7);


mixed indentation

thomasnordquist · 2018-12-03T23:03:02Z

lib/eq3interface.js

+    setDay: (day) => {
+      var out = new Buffer(16);
+      out[0] = 0x10;
+      out[1] = day.day;


mixed indentation

btsimonh · 2018-12-07T08:22:29Z

@thomasnordquist - ready for re-review :).
p.s. the NR node itself is ineffective for me at the moment because of the quantity of devices I have (10).
But would be good to test the node itself with these changes, so that @alikh31 can merge with confidence.

my development flow is here:
https://gist.github.com/btsimonh/178e1e736c7e4a8b0a549991c9331d7c
note that I have 'require' in my settings file (live on the edge!).
This flow is by no means final, but what it attempts to do is:
Only connect to one device at a time (to get over BLE connection limits on platforms).
Serialise all commands.
You can ignore the mqtt aspects for now; but I will want to use the 'eq3interface' with BLE data over MQTT in the end.
I'm on RPi3 with latest(?) bluez (pain in the a**e).
I'm considering something like: shmuelzon/esp32-ble2mqtt#6
but esp32 too unstable for the moment.

Off topic, but I can highly recommend this thermometer for ease of BLE integration, unless you are concerned about data security... (info is sent in advertisment, so simple, and no connection required).

thomasnordquist · 2018-12-10T17:23:34Z

I installed your branch and everything checks out so far.
I'd like to extend the functionality of the actual node-red node as well, so expect a PR on your branch later today 😅.

thomasnordquist

Some more review and a potential "bug"

thomasnordquist · 2018-12-10T15:54:56Z

lib/eq3device.js

 }
+
+// +-7 degrees


I couldn't resist

Suggested change

// +-7 degrees

// temerature offset ranging from -7 to +7 degrees celsius

agree; expect in your pull request :)

thomasnordquist · 2018-12-10T15:59:34Z

lib/eq3interface.js

+
+  // read any return, and convert to a javascript structure
+  // main oare function, which then defers to fiunctions above as required.
+  parseInfo: function(info) {


Nice that you reduced this function to pattern-matching only! 👍

thomasnordquist · 2018-12-10T16:21:34Z

lib/eq3interface.js

 module.exports = {
  writeCharacteristic: '3fa4585ace4a3baddb4bb8df8179ea09',
  notificationCharacteristic: 'd0e8434dcd290996af416c90f4e0eb2a',
  serviceUuid: '3e135142654f9090134aa6ff5bb77046',
  payload: {
-    getInfo: () => new Buffer('03', 'hex'),
+    getSysInfo: () => new Buffer('00', 'hex'), // note change from 03 - 03 is set date, and was RESETTING date every call.


A comment why something has changed, should, in my opinion, be the responsibility the commit message which introduced the change. Referring to something that is no longer part of the code serves no purpose.

It's perfectly fine not to have the comment here, I only marked the place in the review because I was not able to explain the change with my limited knowledge.

setDatetime also documents the 0x03 code.

I agree, the comment serves no purpose if you did not know is was getInfo before.... pls remove in pull request :)

thomasnordquist · 2018-12-10T16:35:11Z

lib/eq3interface.js

        dst: (statusMask & status.dst) === status.dst,
        openWindow: (statusMask & status.openWindow) === status.openWindow,
        lowBattery: (statusMask & status.lowBattery) === status.lowBattery,
+        valvePosition,
+        targetTemperature,


Any reason to duplicatevalvePosition and targetTemperature ?

node-red-contrib-eq3-bluetooth/lib/eq3interface.js

Lines 173 to 181 in e27dfe7

lowBattery: (statusMask & status.lowBattery) === status.lowBattery,

valvePosition,

targetTemperature,

ecoendtime: ecoendtime,

},

valvePosition,

targetTemperature,

};

},

ahh.. just so as not to break someone else who was using the output; yet they were 'in the wrong place' in my thinking, so I wanted to move them gracefully.

I'll see your point. Nothing wrong with changing things. Probably it was not wise to name the group status since everything here can be considered status 🤨
I see no harm if it stays how it is.

thomasnordquist · 2018-12-10T16:39:02Z

lib/eq3device.js

 EQ3BLE.prototype.getInfo = function() {
-  return this.writeAndGetNotification(eq3interface.payload.getInfo())
-  .then(info => eq3interface.parseInfo(info))
+  return this.writeAndGetNotification(eq3interface.payload.setDatetime(new Date()))


breaks setDateTime(date)?!

setting the date will overwritten everytime one tries to get the info.

Is this still true since you changed the getInfo buffer from 0x03 to 0x00?

// this sets the date; else the date can get set to old data in the buffer!!!

with the right date? not sure how else to trigger the status :). just copying the android app....

I see, let me get back to you. I had some very nice reverse-engineered documents from someone else somewhere.

All right, I believe the engineers made the decision to update the thermostats clock whenever a phone is connected. Mixing responsibilities of functions is not a very nice thing to do...

By the way, the documentation I mentioned: https://github.com/Heckie75/eQ-3-radiator-thermostat/blob/master/eq-3-radiator-thermostat-api.md

nice documentation. I'm sure we don't cover it all.

thomasnordquist · 2018-12-10T17:30:20Z

By the way, nice thermometer. Even if it had no bluetooth 😅
I'll definitely order one.

btsimonh · 2018-12-10T17:47:30Z

p.s. my dev flow has a fatal (2 clicks and you are dead fatal) flaw.
Referencing noble devices in global in NR 0.19 is BAD for your NR health. They added a 'Context' tab, and it loses ~30Mbytes per browser refresh.
new one here
ref in forum here

thomasnordquist · 2018-12-10T17:52:51Z

Can you narrow the problem down? Might be more helpful for the devs if they had something reproducible to toy with. Something like a minimal example without hardware requirements?

btsimonh · 2018-12-10T17:59:02Z

I tried, but the noble device is a VERY complex thing. could not reproduce with a simpler (very deep) structure. As I state in the forum, it could be just that the first time it actually fails, and breaks from then on. But it's gonna kill me when I update all my main stuff; I think the avoidance strategy is best; I do some evil stuff with contexts at the moment, all trying to avoid large messages (see 'clone'). To that forum message, I got one reply; I don't think others abuse NR as I do....!

thomasnordquist · 2018-12-10T18:09:29Z

Do you maybe have a cyclic reference?
Some tree traversing algorithms don't check for cycles and therefore crash due to a stack overflow / limited memory.
You could try to store something cyclic in the context and see if it crashes 🤨

var circularReference = {};
circularReference.circularReference = circularReference;

JSON.stringify(circularReference) // FAILS because it can detect cycles, and json doesn't support them

btsimonh · 2018-12-10T18:12:10Z

yep, tested with that :). browser shows 'Circular', and is fine. it's just one to avoid; there is not value having a large structure in the displayed context; it's great for 'normal' people :).

thomasnordquist · 2018-12-10T18:50:34Z

Do you have info on how to use https://github.com/btsimonh/node-red-contrib-eq3-bluetooth/blob/e27dfe72b1d3a1f851d8d05c96e3332f4466a522/lib/eq3interface.js#L36 ?

I'm documenting the node and how to use it, but the description is rather confusing:
https://github.com/btsimonh/node-red-contrib-eq3-bluetooth/blob/e27dfe72b1d3a1f851d8d05c96e3332f4466a522/lib/eq3device.js#L120-L127

btsimonh · 2018-12-10T19:08:42Z

no, didn't play with it too much; so much guessing. The docs you highlighted are quite good though.....

thomasnordquist · 2018-12-25T01:02:37Z

lib/eq3interface.js

@@ -11,60 +11,301 @@ const status = {
  lowBattery: 128,
 }

+// convert any number to 2 digits hex.
+// ensures integer, and takes last two digits of hex conversion with zero filling to 2 if < 16
+var h2 = function(val) {


Is broken

Suggested change

var h2 = function(val) {

var h2 = function(number) {

hah, never trust an englishman! Had a suspicion that something was up with my rads; now I know - the time has probably been s****d for a month! (actually, date/time seems correct; but I've not run anything more complex than polling since the changes....)

thomasnordquist · 2018-12-25T12:16:44Z

Setting the temperature was no longer possible. If you poll a lot, you might want to take a look at influxdb and grafana und 😉

alikh31 · 2019-01-23T11:11:53Z

Hi @btsimonh and @thomasnordquist sorry I've been away for a while. again since I have no physical devices I can not actually test the code but it looks legit to me. if you guys are happy with it I can merge the pr and release the package.
btw since I am not highly available these days if you guys would like to be added as repo admin let me know so I add you guys.

janvda · 2019-03-24T16:22:29Z

Hi @btsimonh ,

I am currently testing branch btsimonh/node-red-contrib-eq3-bluetooth#btsimonh
So it is working very well. Thanks a lot for that.

There are some minor glitches (like "list of all available addressess will be retrieved..." never returning a list) which I want to report as issue on your repository.
But currently I don't have the privilege to report any issue on your repository.
So it would be good if you could enable that feature for others.

kr
Jan

fix setDay (broke if mins=0); add sendRaw function

1f1e019

Add raw data to parse error.

c11a1dc

thomasnordquist reviewed Dec 3, 2018

View reviewed changes

btsimonh added 4 commits December 7, 2018 07:10

update parse function to split into functions

f295f0b

introduce simple number to 2 digit hex function h2(val), and use it.

0a87d1f

online Beautify to 2 spaces, and fix h2().

8992f8d

add rawnotes.txt, some raw notes on observations of protocol.

e27dfe7

thomasnordquist reviewed Dec 10, 2018

View reviewed changes

thomasnordquist reviewed Dec 25, 2018

View reviewed changes

fix a stupid mistake! thanks thomas

be95682

	// +-7 degrees
	// temerature offset ranging from -7 to +7 degrees celsius

	lowBattery: (statusMask & status.lowBattery) === status.lowBattery,
	valvePosition,
	targetTemperature,
	ecoendtime: ecoendtime,
	},
	valvePosition,
	targetTemperature,
	};
	},

eq3device/eq3interface: #12

Are you sure you want to change the base?

eq3device/eq3interface: #12

Conversation

btsimonh commented Nov 3, 2018

btsimonh commented Nov 3, 2018

btsimonh commented Nov 3, 2018

alikh31 commented Nov 3, 2018

btsimonh commented Nov 3, 2018

alikh31 commented Nov 8, 2018

btsimonh commented Nov 8, 2018

thomasnordquist left a comment • edited Loading

Choose a reason for hiding this comment

thomasnordquist Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasnordquist Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

thomasnordquist Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btsimonh commented Dec 7, 2018

thomasnordquist commented Dec 10, 2018

thomasnordquist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasnordquist Dec 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breaks setDateTime(date)?!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasnordquist commented Dec 10, 2018

btsimonh commented Dec 10, 2018

thomasnordquist commented Dec 10, 2018

btsimonh commented Dec 10, 2018

thomasnordquist commented Dec 10, 2018

btsimonh commented Dec 10, 2018

thomasnordquist commented Dec 10, 2018

btsimonh commented Dec 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasnordquist commented Dec 25, 2018 via email

alikh31 commented Jan 23, 2019

janvda commented Mar 24, 2019

thomasnordquist left a comment •

edited

Loading

thomasnordquist Dec 3, 2018 •

edited

Loading

thomasnordquist Dec 3, 2018 •

edited

Loading

thomasnordquist Dec 3, 2018 •

edited

Loading

thomasnordquist Dec 10, 2018 •

edited

Loading

breaks `setDateTime(date)`?!