fix(mdns): Fix compile error when MDNS_PREDEF_NETIF_ETH is defined, but ETH_ENABLED is not (IDFGH-11868)

[marekmaskarinec]

This is a fix for the issue #459.

[CLAassistant]

All committers have signed the CLA.

[david-cermak]

Thanks for posting the fix!

I think that we shouldn't try to handle Ethernet events if CONFIG_MDNS_PREDEF_NETIF_ETH=n.
Would this change fix the issue?

diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c
index 1149ff7..2c95bdc 100644
--- a/components/mdns/mdns.c
+++ b/components/mdns/mdns.c
@@ -3925,7 +3925,7 @@ void mdns_preset_if_handle_system_event(void *arg, esp_event_base_t event_base,
             break;
         }
     }
-#if CONFIG_ETH_ENABLED
+#if CONFIG_ETH_ENABLED && CONFIG_MDNS_PREDEF_NETIF_ETH
     else if (event_base == ETH_EVENT) {
         switch (event_id) {
         case ETHERNET_EVENT_CONNECTED:

[marekmaskarinec]

I didn't do this because I thought the user could add their own Ethernet netif, even if the predefined one is disabled. Should I add this?

[david-cermak]

I thought the user could add their own Ethernet netif, even if the predefined one is disabled.

Yes, this is the idea, but if users add their own interfaces, they have to take care of event handling in the application code (as the library couldn't possibly handle status changes of various netifs which are under user control), there's a convenient API provided just for this purpose:

esp-protocols/components/mdns/include/mdns.h

Lines 812 to 831 in 5ab699d

	/**
	* @brief Set esp_netif to a desired state, or perform a desired action, such as enable/disable this interface
	* or send announcement packets to this netif
	*
	* * This function is used to enable (probe, resolve conflicts and announce), announce, or disable (send bye) mDNS
	* services on the specified network interface.
	* * This function must be called if users registers a specific interface using mdns_register_netif()
	* to enable mDNS services on that interface.
	* * This function could be used in IP/connection event handlers to automatically enable/announce mDNS services
	* when network properties change and/or disable them on disconnection.
	*
	* @param esp_netif Pointer to esp-netif interface
	* @param event_action Disable/Enable/Announce on this interface over IPv4/IPv6 protocol.
	* Actions enumerated in mdns_event_actions_t type.
	* @return
	* - ESP_OK success
	* - ESP_ERR_INVALID_STATE mDNS is not running or this netif is not registered
	* - ESP_ERR_NO_MEM memory error
	*/
	esp_err_t mdns_netif_action(esp_netif_t *esp_netif, mdns_event_actions_t event_action);

The role of predefined interfaces is a legacy handling of the default netifs that were present (hardcoded) in older IDF versions.

Should I add this?

If you're willing to address this issue, I'd suggest you update your commit accordingly. (but only if you feel like you'd like to contribute, which is much appreciated :-)

[marekmaskarinec]

I think that we shouldn't try to handle Ethernet events if CONFIG_MDNS_PREDEF_NETIF_ETH=n.

Done.

[david-cermak]

Thanks for the effort! One minor nitpick, LGTM otherwise.

[david-cermak]

Saw that you had pushed from dce4339 to e529bee, but there's no diff in the code.

Also please note that we've got some trouble with our CI, most of the tests are failing (should be fixed soon).
(This one however relies on the Ethernet module dependency -- should be fixed once you address the last comment)

[marekmaskarinec]

Sorry, I seem to have committed incorrectly. It's fixed now.

[david-cermak]

Thanks for the contribution!