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

Custom Proxy Filter initialization #390

Merged
merged 5 commits into from
Apr 7, 2022
Merged

Conversation

philips77
Copy link
Member

@philips77 philips77 commented Dec 16, 2021

This PR tries to solve #386 in a way proposed in #386 (comment).

Alternative solution have been proposed in #387 and #388.

Pros of proposed solution:

  • Does not require extending ProxyFilter with own implementation, and still allows setting .inclusionList or .exclusionList with custom addresses.
  • Comparing to original version, a custom init state of Proxy Filter can be set.

Cons:

  • I can imaging a custom implementation can have more features.

Breaking change

  • The proxyFilter member of MeshNetworkManager has been changed from Optional to Non-Optional.

Other

  • A bug has been fixed where network manager and proxy filter had strong references to each other.

@philips77
Copy link
Member Author

@bspinner @Eklow02 @adriansergheev
Any comments? There are now 3 PRs solving the same issue :)

@philips77
Copy link
Member Author

Note: This has not been tested yet, as I don't have my DK with me today.

@adriansergheev
Copy link

I could not figure it out, but it seems that it is still needed to call setType on ProxyFilter instance which is not optimal. I don't see at a quick glance how to make it initialized with .exclusionList for example,
Thanks,

@philips77
Copy link
Member Author

philips77 commented Dec 17, 2021

let manager = MeshNetworkManager(...)
manager.proxyFilter.initialState = .exclusionList([])

public var initialState: ProxyFilterSetup = .automatic

@philips77
Copy link
Member Author

The initial state can be changed many times and it's read each time the phone connects to a new Proxy Node.

@adriansergheev
Copy link

adriansergheev commented Dec 17, 2021

thanks a lot for the tip, I thought I saw private(set) in there,
tested with empty exclusion list, works as expected.

@bspinner
Copy link
Contributor

bspinner commented Feb 28, 2022

@philips77 Sorry for late reply.

We are just about to up our usage of group addresses and will have to change the proxy filter more often.
Being able to do so without having to reconnect would be nice.
Since I don't know whether this is a special requirement or not, I would welcome the ability to define custom proxy filter behaviour, as implemented in #388.

@philips77
Copy link
Member Author

With the proposed changes you may set initial setup to required value and change it whenever you want. It will be applied to each new connected node after the setup has changed.
During a connection you may modify the proxy filter with already existing methods, like add or remove.

I understand you need to know when the proxy gets disconnected/a new proxy gets connected so you could assign the right list?

@bspinner
Copy link
Contributor

bspinner commented Mar 1, 2022

Ah, sorry, I forgot about add/remove. I'll try to integrate and test this PR with our app.

@bspinner
Copy link
Contributor

bspinner commented Mar 2, 2022

@philips77 I tried to migrate to this solution today. One thing I'm missing from #388 is a new callback of ProxyFilterDelegate:

https://github.com/bspinner/IOS-nRF-Mesh-Library/blob/allow_custom_proxyfilter_implementations/nRFMeshProvision/Classes/ProxyFilter/ProxyFilter.swift#L66

It gets called when the proxy node acknowledged the proxy configuration message.
If I recall correctly, the already existing proxyFilterUpdated callback is triggered earlier, i.e. when the message has been sent but not yet acknowledged.

In my PR the new callback is called in two places:
https://github.com/bspinner/IOS-nRF-Mesh-Library/blob/allow_custom_proxyfilter_implementations/nRFMeshProvision/Classes/ProxyFilter/DefaultProxyFilter.swift#L299
https://github.com/bspinner/IOS-nRF-Mesh-Library/blob/allow_custom_proxyfilter_implementations/nRFMeshProvision/Classes/ProxyFilter/DefaultProxyFilter.swift#L334

I didn't want to break behaviour for existing users of ProxyFilterDelegate and therefore added a function.
So maybe we can just modify handling of proxyFilterUpdated instead of adding a new callback?

@bspinner
Copy link
Contributor

@philips77 Any thoughts?

@philips77
Copy link
Member Author

Hello, sorry, I was busy with other project. I don't see reason why not to add such callback.
I'll resume working on mesh in few weeks, hopefully. You may try creating a PR to this PR with desired changes, which may be quicker.

bspinner and others added 2 commits March 28, 2022 11:20
Don't pass addresses but listSize, as we don't know the addresses for sure.
Don't call acknowledged callback when proxy filter received unexpected addresses.
Adds proxyFilterUpdateAcknowledged delegate
@philips77
Copy link
Member Author

OK, I'm on it. I shall release this week.

@philips77 philips77 merged commit 13b52b5 into main Apr 7, 2022
@philips77 philips77 deleted the feature/proxy-filter-init branch April 7, 2022 11:37
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.

3 participants