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

[ESP32] Subscription Persistence and Resumption don't work on ESP32 #25013

Closed
wqx6 opened this issue Feb 13, 2023 · 17 comments · Fixed by #25173
Closed

[ESP32] Subscription Persistence and Resumption don't work on ESP32 #25013

wqx6 opened this issue Feb 13, 2023 · 17 comments · Fixed by #25173
Labels
icd Intermittently Connected Devices

Comments

@wqx6
Copy link
Contributor

wqx6 commented Feb 13, 2023

Reproduction steps

Subscription Persistence and Resumption don't work on ESP32.
My test steps:

# Pairing the device
./chip-tool interactive mode
> pairing ble-wifi 1 ssid password 20202021 3840
# Send subscribe command
> onoff subscribe on-off 10 100 1 1 --keepSubscriptions 1
# Reboot the device, the subscription reports cannot be received in chip-tool.

chip-tool.log
esp32.log

Bug prevalence

Whenever I do this

GitHub hash of the SDK that was being used

bbfd5d6

Platform

esp32

Platform Version(s)

No response

Anything else?

No response

@bzbarsky-apple
Copy link
Contributor

@wqx6 Please attach (not paste) logs?

@mkardous-silabs
Copy link
Contributor

Basic question, is the app built with the persistent subscription flag on?

@bzbarsky-apple bzbarsky-apple added the icd Intermittently Connected Devices label Feb 13, 2023
@wqx6
Copy link
Contributor Author

wqx6 commented Feb 14, 2023

@wqx6 Please attach (not paste) logs?

Attached the logs.

@wqx6
Copy link
Contributor Author

wqx6 commented Feb 14, 2023

Basic question, is the app built with the persistent subscription flag on?

the persistent subscription flag should be on after #24759

@bzbarsky-apple
Copy link
Contributor

So for the server log, I do see:

I (892) chip[SVR]: Initializing subscription resumption storage...

which indicates that subscription resumption is generally present.

@bzbarsky-apple
Copy link
Contributor

This is fallout from #24725

During server startup, MinMdnsResolver::IsInitialized is returning false, and then we never end up trying to restore things. Looks like we don't bring up the minmdns bits until we have established connectivity (not suprising!), but this happens quite a bit after server startup....

@bzbarsky-apple
Copy link
Contributor

@Damian-Nordic So... When kDnssdPlatformInitialized happens, that calls StartServer(), so posting that event from inside when minimal mdns starts to listen is wrong: we will just end up restarting the server from the event, etc.

I guess we could change examples/platform/esp32/common/CommonDeviceCallbacks.cpp to post this event instead of StartServer, but that seems pretty whack-a-mole: what about other platforms?

@bzbarsky-apple
Copy link
Contributor

And generally speaking, during startup, there will be various events as we bring up various different interfaces (ipv4, ipv6, etc). Do we have some event for "ok, we think our network is now stable"?

@Damian-Nordic
Copy link
Contributor

Damian-Nordic commented Feb 14, 2023

So, I added the following code:

mIsDnssdReady = Dnssd::Resolver::Instance().IsInitialized();

and then:

        // Platform DNS-SD implementation uses kPlatformDnssdInitialized event to signal that it's ready.
        if (!mIsDnssdReady)
        {
            mIsDnssdReady = true;
            CheckServerReadyEvent();
        }

Thus mIsDnssdReady can become true only once. I guess what I assumed (and this is to be verified) is that MinMdnsResolver::Init is synchronous and then MinMdnsResolver::IsInitialized must return true. I assumed so, because the minimal mDNS resolver does not spawn any event like kDnssdPlatformInitialized nor provides a callback. I'll take a look at that.

And generally speaking, during startup, there will be various events as we bring up various different interfaces (ipv4, ipv6, etc). Do we have some event for "ok, we think our network is now stable"?

I think currently different examples use different events to e.g. start the OTA requestor. I envision the kServerReady that I have added to become such kind of event at some point.

@Damian-Nordic
Copy link
Contributor

So I checked on linux with minimal mDNS and that works. That is, MinMdnsResolver::IsInitialized returns true right after MinMdnsResolver::Init. Looking at the code, it may not be true if the network interfaces are not up before initializing the server or an error occurs while trying to listen on those interfaces. @wqx6 or anyone, do you understand why this is different on ESP32? I can add a delegate to the Resolver interface to have a common asynchronous initialization notification mechanism but need to understand the current flow on different platforms.

@wqx6
Copy link
Contributor Author

wqx6 commented Feb 14, 2023

@Damian-Nordic MinMdnsResolver::IsInitialized returns false on ESP32 because the network interfaces are not up before initializing the server, so there is no endpoint created in minimal mdns server. The endpoints are created when calling the DnssdServer::Instance().StartServer()(ESP32 gets IPs).

@Damian-Nordic
Copy link
Contributor

I see. Ok, let me fix that.

@Damian-Nordic
Copy link
Contributor

@wqx6 Could you check if #25050 helps?

@bzbarsky-apple
Copy link
Contributor

See #25050 (review) -- that PR helps, but not enough on its own. But it makes it so things are no more broken than they were before #24725 at least... Still not working, though, @jtung-apple.

Perhaps Server should be listening for some set (which?) of connectivity events and trying to restart subscriptions if they have not already been restarted when those happen?

@Damian-Nordic
Copy link
Contributor

Hmm. Maybe ResumeSubscriptions shouldn't be called by the server, but by the application, similarly to the initialization of the OTA requestor. At least until we figure out how to generate the server initialization event reliably across platforms.

@wqx6
Copy link
Contributor Author

wqx6 commented Feb 15, 2023

@Damian-Nordic Thanks a lot. The PR helps. In my environment, the subscriptions resumption works on ESP32. So in @bzbarsky-apple 's comment, the DnssdInitialized event might be posted even if there is no IPv6 address on WIFI_STA netif? Can we post the event until there is an IPv6 interface used by minimal mdns server? just add a addressType check in if (!mIsInitialized)

bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Feb 17, 2023
…e IPv6 interface.

This fixes two things:

1. We now don't consider advertising properly initialized until we are
   advertising on at least one ipv6 interface.

2. Actually check which sorts of addresses interfaces have, instead of
   just assuming that all interfaces have both IPv4 and IPv6 addresses.

Fixes project-chip#25013
@bzbarsky-apple
Copy link
Contributor

@wqx6 #25173 aims to do that. It fixes the problem for me...

bzbarsky-apple added a commit that referenced this issue Feb 20, 2023
…e IPv6 interface. (#25173)

This fixes two things:

1. We now don't consider advertising properly initialized until we are
   advertising on at least one ipv6 interface.

2. Actually check which sorts of addresses interfaces have, instead of
   just assuming that all interfaces have both IPv4 and IPv6 addresses.

Fixes #25013
kkasperczyk-no pushed a commit to kkasperczyk-no/sdk-connectedhomeip that referenced this issue Mar 15, 2023
…e IPv6 interface. (#25173)

This fixes two things:

1. We now don't consider advertising properly initialized until we are
   advertising on at least one ipv6 interface.

2. Actually check which sorts of addresses interfaces have, instead of
   just assuming that all interfaces have both IPv4 and IPv6 addresses.

Fixes project-chip/connectedhomeip#25013
kkasperczyk-no pushed a commit to kkasperczyk-no/sdk-connectedhomeip that referenced this issue Mar 15, 2023
…e IPv6 interface. (#25173)

This fixes two things:

1. We now don't consider advertising properly initialized until we are
   advertising on at least one ipv6 interface.

2. Actually check which sorts of addresses interfaces have, instead of
   just assuming that all interfaces have both IPv4 and IPv6 addresses.

Fixes project-chip/connectedhomeip#25013
lecndav pushed a commit to lecndav/connectedhomeip that referenced this issue Mar 22, 2023
…e IPv6 interface. (project-chip#25173)

This fixes two things:

1. We now don't consider advertising properly initialized until we are
   advertising on at least one ipv6 interface.

2. Actually check which sorts of addresses interfaces have, instead of
   just assuming that all interfaces have both IPv4 and IPv6 addresses.

Fixes project-chip#25013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
icd Intermittently Connected Devices
Projects
None yet
4 participants