-
Notifications
You must be signed in to change notification settings - Fork 34
Expand discovery filter with model, ID, and brand support #288
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
base: development
Are you sure you want to change the base?
Conversation
Signed-off-by: Adam Vest <[email protected]>
Signed-off-by: Adam Vest <[email protected]>
|
@Ziris85 Thanks for this addition to the discovery filtering. I haven't got the possibility to look at this closer or test it for a few days, just a comment about the added options.
Wouldn't a white-, or possibly more appropriately a black-list have been an easier way to exclude your neighbour's device, not just form being discovered, but also from being published at all to your MQTT broker? I'm just a bit worried that also including the id/MAC address in discovery filtering it might confuse some users in what is the best way for avoiding neighbour's or passerby's devices, with such an id discovery filering not discovering these, but still having them published with their MQTT messages, while a white-list with only your own devices would keep out any devices from others, discovery and published messages, when id discovery filtering supresses the discovery, but the still received MQTT messages might be confusing, then the next device with possibly the same model_id, but different id comes along and will be discovered ... Also with the brand and model, these can and do change occasionally, in the case where additional models or a rebranded brand are being added to an already existing decoder, a good example being the This would then require constant updating for the users of their filters, but alos cause ambiguous filtering of several decoders. Just some quick thoughts about this while on the road ;) best for us to look at this and possibly discuss the different options more closely the next days. |
|
@DigiH , Heh, for my use-case you're probably right since I'm indeed entirely disinterested in the data. For the I'll probably adjust my approach in this case since I was misunderstanding the differences there, but do you think there might still be value in the expanded flexibility with the discovery_filter here? Maybe there could be a blurb in the docs to clarify that the |
Or some such. |
Correct, the white- and black-lists do take the MAC address of the devices making it uniquely clear which are allowed and/or banned from being published. The discovery filter however is a remnant of the time when there were several model_ids which had to be filtered from discovery due to them having constantly changing random MAC addresses or tore reasons. Since the introduction of random MAC address devices tagging, the only practically left decisively filter is for iBeacons, which 95% of the time you wouldn't want to be discovered, but for some you still want to received MQTT messages for manually adding and processing in a controller. And iBeacons really being the only kind of broadcasts where you might not want a discovery, but still might required MQTT messages to be received. For that reason I would actually reject any extension of the discovery filter, but to vote to completely remove it from any user interaction, much like OpenMQTTGateway already does it, only filtering out iBeacons in the background and leaving user side filtering solely to a white-list and/or black-list. @1technophile what is your stance on this? @Ziris85 do you understand my reasoning behind this? One thing which could help is to extend the white- black-list functionality to also include model_ids, so as to be able to allow or restrict whole model-id groups of devices. This would again not be as clearly defined as individual MAC addresses, but could allow quicker adding of same model devices. And once discovery is turned off again after all owned devices ahave been discovered, it wouldn't really matter if a passing by device with the same model_id would the be published via MQTT, but not discovered as discovery is off at that stage. One thing which is sorely needed though is to be able to temporarily turn off a white-or back-list, as currently this is only possible be clearing the whole lists, and then having to populate them again to get the lists back. Something along the lines of OpenMQTTGateway's ignoreWBlist |
|
But all my hard work! lol, yeah that makes sense. We can leave this PR around until @1technophile has had a chance to chime in, but feel free to close it once done 🙂 |
|
Thank you for your work on this PR! I can see the value in the expanded discovery filter. I’m thinking of an alternative approach that could improve flexibility. Instead of introducing separate fields for id, model, and brand, what if we implemented a JSONPath-based filtering system? This approach would allow users to filter based on any key in the MQTT message payload, offering more flexibility than predefined fields. For example: This approach consolidates filtering logic into a single field. To help guide users, we could also add documentation explaining best practices — for example: Also I'm thinking that one filter could be used for discovery and messages instead of having a black list, white list, discovery filter. One drawback of this approach is how to properly write and display a json like this while we are not managing the field GUI. Thoughts on this ? |
Do you mean that the above example "include" and "exclude" would be applicable to both discovery filtering AND white-/black-listing? In that case I would disagree, as iBeacons in general I want to be filtered from discovery, but I'd still want to have some iBeacon beacons white-listed for receiving their broadcasts and messages for Home/Away device and/or room presence tracking. I am also still not sure about the actual benefit of a separate discovery filtering, in addition to the very useful white and black-listing. Like OMG, which doesn't have nay discovery and automatically only filters iBeaons from discovery, without really missing anything from OMG or any users ever asking for any additional discovery filtering option. Maybe unless someone is in a real BLE busy place and even when they turn on discovery very briefly to discovery any new devices they would already also catch some other devices ;) Any expansion of white-list and black-list with the above additional options would be greatly beneficial to both TGW and OMG, I think. |
Yes indeed
Do you see other examples where it is relevant to separate the discovery filtering from the message filtering ?
|
Yes, I agree, iBeacon discovery should be hard coded, just like in OMG. Just like all RMAC defined decoders are. With RMAC defined decoders though, the only real white-listing - and discovery at the same time - is only applicable through the already existing MAC address white-listing, and any other white-listing options might only dd to confusion. Any additional property for white-listing and/or discovery filtering might be very convenient at the beginning, but in the end for white-listing only MAC addresses are a surefire way to uniquely identify your own devices. An exception is black-listing, but only in TGW, not OMG, where both a white-list and black-list can be applied at the same time, to black list whole gourd of model-ids or even brands. White MAC address only, as already implemented, I find the best way go go implement any new devices is to have a white-list with existing devices' AMCs, have discovery turned off, which might also benefit from a default 30 minutes timeout as in OMG, temporarily disable the white-list so as to see the details of any new devices, including its MAC address; add this new MAC address to the white-list; enable the white-list again and turn ion discovery. Just my thoughts :) |
Description:
This is a humble attempt at expanding on the
discovery_filteroption to include more flexibility than just model_id filtering. In my use-case my neighbor happens to have a device that shares the same model_id as mine, which left me with no way to filter their discovery out without filtering mine out as well. This PR expands the option to allow filtering based off of:Ideally I could have made this backward-compatible with the existing way the config is written (since it does require a change there), but I was unable to find a way to work that in. If that's desired and someone has an idea on how to include that, I'll be happy to adjust the PR accordingly!
Checklist: