quartiq / miniconf

Rust serialize/deserialize/access reflection for trees (no_std, no alloc)
MIT License
23 stars 2 forks source link

Interference/problems with miniconf and other mqtt paths #142

Closed nkrackow closed 1 year ago

nkrackow commented 1 year ago

Using the latest master branch with eg. Stabilizer dual-iir (aka using vanilla Stabilizer master branch, just changing the miniconf dep to the github master and minimq to 0.6.2) leads to the following issues:

Miniconf seems to be confused with messages that are not intended for it. I get the following debug prints:

INFO - Starting
INFO - EUI48: 80-34-28-1f-59-65
INFO - setup() complete
WARN - Network link DOWN
WARN - Network link UP
INFO - MQTT connected, subscribing to settings
INFO - Unknown Miniconf command: /alive
INFO - Unknown Miniconf command: /telemetry

If I publish onto a miniconf setting I get WARN - Failed to build response message.

The miniconf settings are applied at the device and I also see the mqtt messages like /alive on the host.

jordens commented 1 year ago

https://github.com/quartiq/miniconf/blob/8be449dcd9be7c838720c21223dac0841595dd26/src/mqtt_client/mod.rs#L324

ryan-summers commented 1 year ago

I think this may be a knocking effect of the new get/set commands and the telemetry prefixes conflicting and I'm not sure this is a defect in miniconf. Perhaps we need to think more about how the mqtt tree should look...

nkrackow commented 1 year ago

Maybe these are even two separate problems. The unknown commands being one and failing to respond to the received settings (WARN - Failed to build response message) the other. I also don't see any response messages on the host.

nkrackow commented 1 year ago

I think this is indeed two separate problems. The unknown miniconf commands are actually valid because miniconf listens for commands on the device path eg. dt/sinara/dual-iir/80-34-28-1f-59-65. If I publish onto /settings or /list there its a valid miniconf command but obviously /alive is not a valid miniconf command. Maybe we should put in an extra subtopic or just take out that info for unknown miniconf commands?

I was able to fix the failed response messages like this: https://github.com/quartiq/miniconf/commit/1e8278c7b41ef6b854c6b0a7edffd76f5d592e39 I don't think this is the most sensible solution but obviously the problem is that there is no ResponseTopic in properties when I publish onto /settings.

jordens commented 1 year ago

Posts to unknown topics at that hierarchy level should just be ignored. But it should not receive the alive message at all in the first place.

jordens commented 1 year ago

If there is no response topic ist should not try to send a response. No need for a default response topic imo.

nkrackow commented 1 year ago

Well but we used to always get the response if the setting or the path are invalid which was very helpful. And as it is it always wants to send this but doesn't have a response topic.

jordens commented 1 year ago

Might be useful for debugging but it's not useful with the miniconf client nor required by the miniconf protocol. Hence it's also not tested or maintained. Let's get rid of it. The recommendation is to set a response topic (use the miniconf client)

ryan-summers commented 1 year ago

Agreed with @jordens here. The ResponseTopic will provide failure information such as an invalid path or setting. If there's no ResponseTopic specified, I don't think we should be doing anything.