Implement XML Position messages#916
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## dev #916 +/- ##
==========================================
+ Coverage 92.46% 92.50% +0.03%
==========================================
Files 121 122 +1
Lines 4644 4668 +24
Branches 292 294 +2
==========================================
+ Hits 4294 4318 +24
Misses 288 288
Partials 62 62 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #916 will not alter performanceComparing Summary
|
| @pytest.mark.parametrize("position", [(-9, 15, 89)]) | ||
| def test_Pos(position: tuple[int, int, int]) -> None: | ||
| x, y, a = position | ||
| xml_message = f'<ctl td="Pos" t="p" p="{x},{y}" a="{a}" valid="1" />' |
There was a problem hiding this comment.
Some questions about the message:
- is
tthe type and will it be different for the charger position? - There is a
validattribute... I would assume that the position is only valid if it is equal to 1. Can you please verify it
There was a problem hiding this comment.
Fair point
- I have no way of checking if valid = 0 ever happens, but I guess one more check doesn't hurt.
- The app does show the charger position, I just didn't figure out what param it uses. I'll check that tomorrow
There was a problem hiding this comment.
There's a GetChargerPos command, but I didn't find any "Pos" events for it.
Suggestion:
- Imlpement GetChargerPos
- Only handle valid="1" t="p" messages as bot position, we'll leave everything else as HandlingResult.analyse() so that they can be eventually debugged further
| p_x, p_y = p.split(",", 2) | ||
| p_a = xml.attrib.get("a", 0) | ||
| position = Position( | ||
| type=PositionType.DEEBOT, x=int(p_x), y=int(p_y), a=int(p_a) |
There was a problem hiding this comment.
Is the app not showing where the charger is?
There was a problem hiding this comment.
For Deebot 900 it shows the charger position in app.
But it looks like the position is explicitly requested:
Got message: topic=iot/p2p/GetChargerPos/HelperMQClientId-awseu-sts-ngiot-mqsjmq-43/ecosys/<id>/<uuid>/ls1ok3/<..>/q/<...>/x, payload=b'<ctl/>'
[...]
Got message: topic=iot/p2p/GetChargerPos/<uuid>/ls1ok3/<id>/HelperMQClientId-awseu-sts-ngiot-mqsjmq-43/ecosys/<..>/p/<...>/x, payload=b"<ctl ret='ok' p='-200,816' a='-90'/>"
Edit: ahh, noticed that @nanomad already replied in the other comment
nanomad
force-pushed
the
feature/xml-messages-pos
branch
2 times, most recently
from
8dc8739 to
a64ff99
Compare
| """Support class for producing Pos events.""" | ||
|
|
||
| @classmethod | ||
| def _parse_xml( |
There was a problem hiding this comment.
Move this function to the Pos message. No need for an additional class
| return HandlingResult.analyse() | ||
|
|
||
|
|
||
| class GetPos(XmlCommandWithMessageHandling, PosParser): |
There was a problem hiding this comment.
| class GetPos(XmlCommandWithMessageHandling, PosParser): | |
| class GetPos(XmlCommandWithMessageHandling, Pos): |
|
|
||
| return HandlingResult.analyse() | ||
|
|
||
| class GetChargerPos(XmlCommandWithMessageHandling, PosParser): |
There was a problem hiding this comment.
| class GetChargerPos(XmlCommandWithMessageHandling, PosParser): | |
| class GetChargerPos(XmlCommandWithMessageHandling, Pos): |
| cls, position_type: PositionType, event_bus: EventBus, xml: Element | ||
| ) -> HandlingResult: | ||
| """Handle xml message and notify the correct event subscribers.""" | ||
| if (p := xml.attrib.get("p")) and (xml.attrib.get("valid", "1")) == "1": |
There was a problem hiding this comment.
| if (p := xml.attrib.get("p")) and (xml.attrib.get("valid", "1")) == "1": | |
| if (p := xml.attrib.get("p")) and xml.attrib.get("valid") == "1": |
We currently have tests, where always valid is present. Therefore we should analyse the message, when its missing instead assuming it's valid
There was a problem hiding this comment.
valid=1 is missing from GetChargerPos though, I can either move the check to GetPos and Pos and remove it from the parser or assume it's "1" if the attribute is missing
nanomad
force-pushed
the
feature/xml-messages-pos
branch
from
aa14cca to
a8efe15
Compare
3 participants
No description provided.