Add acceleration values and a few others, plus unit test improvements #29
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #29 +/- ##
===========================================
+ Coverage 70.94% 98.95% +28.01%
===========================================
Files 5 6 +1
Lines 117 287 +170
Branches 11 24 +13
===========================================
+ Hits 83 284 +201
+ Misses 28 3 -25
+ Partials 6 0 -6 ☔ View full report in Codecov by Sentry. |
|
I am running this along w/the new values on 15 ruuvi tags, 6 running the V3 and 9 running the V5. Also I will be unavailable until Monday so will check out any comments then. |
There was a problem hiding this comment.
Couple inline suggestions - it would probably make sense to rebase this on top of the current main here instead of merging it in halfway through to get rid of some of the spurious-looking changes?
Secondly, I hope there's an easy way in HA to disable some of these entities; I wouldn't need my tags to necessarily record accelerations, etc. all the time (and save them in the Recorder database) 🤔
Also, Mypy is complaining about something here.
Have a nice weekend! 😄 (I'm off to have some birthday beers in a bit, myself...)
src/ruuvitag_ble/df5_decoder.py
Outdated
| ax, ay, az = self.acceleration_vector_mg | ||
| if ax is None or ay is None or az is None: | ||
| return None | ||
| return math.sqrt(ax * ax + ay * ay + az * az) | ||
| # Conversion to m/s^2 | ||
| return round(math.sqrt(ax * ax + ay * ay + az * az) / 1000.0 * 9.8, 2) |
There was a problem hiding this comment.
Let's use math.hypot here to let CPython do more of the work.
|
|
||
| @property | ||
| def mac(self) -> str: | ||
| return ":".join(f"{x:02X}" for x in self.data[10:]) |
There was a problem hiding this comment.
I didn't see much point in extracting this from the advertisement when it's already available in the service data. I don't think it's possible for them to be different. This goes with the removal of its use in the parser.
| # (preferring the MAC address the tag broadcasts). | ||
| identifier = short_address(decoder.mac or service_info.address) | ||
| identifier = short_address(service_info.address) |
There was a problem hiding this comment.
This looks like a spurious change?
There was a problem hiding this comment.
This goes with the removal of the mac explained above. It seems cleaner to me to just use the service info and is one less thing to parse. Unless you know of some reason the one extracted from the advertisement is preferred?
src/ruuvitag_ble/df3_decoder.py
Outdated
| ax, ay, az = self.acceleration_vector_mg | ||
| if ax is None or ay is None or az is None: | ||
| return None | ||
| return math.sqrt(ax * ax + ay * ay + az * az) | ||
| # Conversion from milliG to m/s^2 | ||
| return round(math.sqrt(ax * ax + ay * ay + az * az) / 1000.0 * 9.8, 2) |
There was a problem hiding this comment.
Let's use math.hypot here to let CPython do more of the work.
| advertisement = bytes_to_service_info(V5_OUTDOOR_SENSOR_DATA) | ||
| advertisement = bytes_to_service_info(V5_SENSOR_DATA) | ||
| assert device.supported(advertisement) | ||
| up = device.update(advertisement) | ||
| expected_name = "RuuviTag EFAF" | ||
| expected_name = "RuuviTag DCFE" |
There was a problem hiding this comment.
These seem like spurious changes too..?
There was a problem hiding this comment.
I guess as mentioned in the previous MR I wasn't really sure why "OUTDOOR" needed to be specified. The parser doesn't care if data is outdoor or indoor, the unit tests only need verify that the outputs match the inputs. I added a more "diverse" input in the V3 test but it'd be easy for me to generate a few more based on the tags I have if you think that's necessary (I have some in a freezer, some in a hot attic, etc). I think we're pretty well covered at this point though. The MAC change is because the original test had a mac of 00:00:00:00:00:00 and I didn't think that was a very good test (and also the removal of the in advertisement mac parser).
Yeah I will summon up my git fu and try and get this done once I fix the other comments.
That part I don't understand quite as well- I noticed that the new entities were disabled by default, I had to go enable them. If they're disabled do they still get recorded? Even if so it's just an extra couple of bytes I think (the rate of the tag's advertisements shouldn't change).
Yeah I am looking at that. It's whining about me adding a pytest import to test the functions that raise... I will keep looking.
Excellent! Love me some birthday beers :) |
This PR adds the individual acceleration values. These are super handy because they help tell you the orientation of the sensor. E.g. if the sensor is on its side the Z axis will report acceleration close to zero, whereas when the sensor is lying flat it will report acceleration close to 9.8 m/s^2 (generally 1G).
Also bubbled up now are the tx power and sequence counters on the V5 version of the firmware.
Lastly with this PR comes some unit test improvments and code coverage improvements.