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

Add Xiaomi RoPot device #105

Merged
merged 1 commit into from
Mar 29, 2022
Merged

Add Xiaomi RoPot device #105

merged 1 commit into from
Mar 29, 2022

Conversation

emericg
Copy link
Contributor

@emericg emericg commented Mar 28, 2022

Description:

Add Xiaomi RoPot device.
Let me know if the position of the RoPot in the _devices array is not good for you.

Checklist:

  • The pull request is done against the latest development branch
  • Only one feature/fix was added per PR and the code change compiles without warnings
  • I accept the DCO.

@1technophile
Copy link
Member

Thanks! Could you add also some test case like the MiFlora please,
Here is the expected json

"{\"brand\":\"Xiaomi\",\"model\":\"Miflora\",\"model_id\":\"HHCCJCY01HHCC\",\"fer\":0}",

And here is the input data:

{"Mi flora", "712098000863b6658d7cc40d0910020000"},

You will need to add the enum below also:

TheengsDecoder::BLE_ID_NUM::HHCCJCY01HHCC,

@emericg emericg force-pushed the ropot branch 2 times, most recently from 6709f1b to 6fb16a6 Compare March 29, 2022 15:42
@DigiH
Copy link
Member

DigiH commented Mar 29, 2022

Thanks @emericg

note the test erros

Expected: {"brand":"Xiaomi","model":"RoPot","model_id":"HHCCPOT002","moi":3}
Got JSON: {"brand":"HHCC","model":"Ropot","model_id":"HHCCPOT002","moi":3}

differnce between your commented out nested definition vs. onst char* _HHCCPOT002_json

also please have a look at my comment about the model condition. The decoder still has a battery property in _HHCCPOT002_json_props which it actually doesn't seem to get from the data.

@emericg
Copy link
Contributor Author

emericg commented Mar 29, 2022

Ok I've fixed the enum, and added test cases.
As far as I can see I don't have any temperature coming from the RoPot advertisement, but I'm maybe just unlucky. Anyway it's a standart MiBeacon procotol so worst case we can remove it later.

@DigiH
Copy link
Member

DigiH commented Mar 29, 2022

Still the upper/lower case issue with "model"

Expected: {"brand":"Xiaomi","model":"RoPot","model_id":"HHCCPOT002","moi":3}
Got JSON: {"brand":"Xiaomi","model":"Ropot","model_id":"HHCCPOT002","moi":3}

@emericg
Copy link
Contributor Author

emericg commented Mar 29, 2022

Daamn. I can push this PR into one commit if you prefer.

@DigiH
Copy link
Member

DigiH commented Mar 29, 2022

Daamn. I can push this PR into one commit if you prefer.

That would be preferable, but first please have a look at my comment about the model condition above, can you see it, about it currently only being

"condition":["servicedata", "contain", "5d01"],

which can easily catch wrong devices which have a temperture of 34,9 ºC for example, or steps for the Mi Band etc. So a more specific

"condition":[""servicedata", "index", 4, "5d01"],

will avoid this.

@emericg
Copy link
Contributor Author

emericg commented Mar 29, 2022

Sure no problem. This could be applied to the flower care as well, the 2 bytes starting at offset 4 are the product ID.
The flower care check for a '0x02' byte before the product ID, but I'm not sure what is it (and I have an older flower care that use a 0x20 instead).

@DigiH
Copy link
Member

DigiH commented Mar 29, 2022

Sure no problem. This could be applied to the flower care as well, the 2 bytes starting at offset 4 are the product ID.
The flower care check for a '0x02' byte before the product ID, but I'm not sure what is it (and I have an older flower care that use a 0x20 instead).

Probably best to stick with the ones you can verify with your devices for the moment. So getting the product ID 2 bytes static at index 4, great. Also, do you get a serviceuuid (when you add '-DpubBLEServiceUUID=true') to your build flags or possibly also a name when you currently monitor the published MQTT message with the servicedata?

@DigiH
Copy link
Member

DigiH commented Mar 29, 2022

Looking good.

I've seen your WatchFlower project - great to get some of those extra device into the Decoder here :)

Thanks

@DigiH DigiH merged commit d431605 into theengs:development Mar 29, 2022
@emericg
Copy link
Contributor Author

emericg commented Mar 29, 2022

Sure no problem. This could be applied to the flower care as well, the 2 bytes starting at offset 4 are the product ID.
The flower care check for a '0x02' byte before the product ID, but I'm not sure what is it (and I have an older flower care that use a 0x20 instead).

Probably best to stick with the ones you can verify with your devices for the moment. So getting the product ID 2 bytes static at index 4, great. Also, do you get a serviceuuid (when you add '-DpubBLEServiceUUID=true') to your build flags or possibly also a name when you currently monitor the published MQTT message with the servicedata?

I have all the devices :p An old Flower Care with 0x20 (but it has no useful data) and a new one with 0x02.

{"id":"C4:7C:8D:6D:0C:D2","name":"ropot","servicedata":"71205d0106d20c6d8d7cc40d0910020000","brand":"Xiaomi","model":"RoPot","model_id":"HHCCPOT002","fer":0}
{"id":"C4:7C:8D:62:D4:BE","name":"Flower care","servicedata":"310298004abed4628d7cc40d"}
{"id":"C4:7C:8D:66:D3:B7","name":"Flower care","servicedata":"712098000bb7d3668d7cc40d041002fb00","brand":"Xiaomi","model":"Miflora","model_id":"HHCCJCY01HHCC","tempc":25.1,"tempf":77.18}

And the 16b service UUID is 0xFE95, like the flower care.

@DigiH
Copy link
Member

DigiH commented Mar 29, 2022

I have all the devices :p An old Flower Care with 0x20 (but it has no useful data) and a new one with 0x02.

And the 16b service UUID is 0xFE95, like the flower care.

The 'new' Flower Care already is integrated with the Mi Flora decoder, also with a "contain" only, but a more distinct 3 byte.

@1technophile @h2zero - wondering if this maybe was implemented before the "index" option, and if an additional uuid comparison could/should be included?

Same for the just merged RoPot decoder, which is more distinct now with the index condition, but would be even more unique with an addition AND uuid, which would then also require for the tests to be moved to the test_uuid section.

An old Flower Care with 0x20 (but it has no useful data)

@emericg the only different bytes I can obviously see here, apart from the product ID, are

{"id":"C4:7C:8D:62:D4:BE","name":"Flower care","servicedata":"310298004abed4628d7cc40d"}

so not sure if you see them changing over time, or if the old model is only giving it's details with a connection ;)

@h2zero
Copy link
Member

h2zero commented Mar 29, 2022

@1technophile @h2zero - wondering if this maybe was implemented before the "index" option, and if an additional uuid comparison could/should be included?

If I recall correctly, this decoder was derived from the original code in OMG.

If a better/more specific detection is available then it should probably be added.

@DigiH
Copy link
Member

DigiH commented Mar 29, 2022

I'm just thinking, that while a "contain" with 3 bytes looks quite unique for most cases, it could still, under some conditions, catch another device with matching data anywhere. So restricting it with "index" gives a higher uniqueness, and any addition serviceuuid would be ideal.

There are quite a few, albeit all with 3 byte

"servicedata", "contain",

only model condition decoders which could benefit from this.\

P.S.: So a very active Mi Band user with 38944 steps will currently be recognised as a Mi Flora device LOL

@DigiH
Copy link
Member

DigiH commented Mar 29, 2022

I can make these "index" changes tomorrow if you all agree.

@emericg - shall I extend the RoPot condition to the 3 byte "205d01" at the same time?

@emericg
Copy link
Contributor Author

emericg commented Mar 29, 2022

I have no particular opinion on the 0x20.

However if you do that, I think you can also remove the temperature from the RoPot. I still haven't got a temp reading, and I found out that esphome wasn't including it either, just moisture and fertility.
Also from what I gathered a couple days ago, the temperature is never included in the history data (which is very weird but... seems consistent with other observations now) so it seems to point to the temperature being treated differently with the RoPot.

With a direct connection I can read the current temperature and battery level though. But because these are not included in the theengs properties they are going to be ignored anyway.

@DigiH
Copy link
Member

DigiH commented Mar 30, 2022

I've extended it to

"condition":[""servicedata", "index", 2, "205d01"],

and also removed the temperature according to your info. So that and the battery are not possible through the Decoder, but through OMG READ commands directly.

Thanks

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

Successfully merging this pull request may close these issues.

4 participants