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

feat(mdns): Make including mdns_console KConfig (IDFGH-11988) #497

Merged

Conversation

rretanubun
Copy link
Contributor

The CLI that mdns_console offers is very useful for debugging and bootstrapping mDNS onto new projects, but some projects may not want console CLI in place to reduce binary size.

This gives a KConfig option to do so.

The CLI that mdns_console offers is very useful for debugging
and bootstrapping mDNS onto new projects, but some projects
may not want console CLI in place to reduce binary size.

This gives a KConfig option to do so.
@CLAassistant
Copy link

CLAassistant commented Jan 26, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot changed the title feat(mdns): Make including mdns_console KConfig feat(mdns): Make including mdns_console KConfig (IDFGH-11988) Jan 26, 2024
@igrr
Copy link
Member

igrr commented Jan 26, 2024

@rretanubun do you see mdns_console contributing to binary size even when not calling mdns_console_register? (You can check with idf.py size* commands.)
I think this should normally not happen, as the linker will eliminate all the unused object files and functions from the resulting binary.

@rretanubun
Copy link
Contributor Author

@igrr : No I do not, the linker does eliminate it. I was thinking adding an upstream supported KConfig option to allow projects to build with

#ifdef CONFIG_MDNS_ENABLE_CONSOLE_CLI
    mdns_console_register();
#endif

then each sdkconfig variant controls if the project builds gets mdns console on the CLI or not and the community going forward all just use the same KConfig name.

@david-cermak
Copy link
Collaborator

I agree with Ivan that this change won't affect the binary size, but may help reduce build time and since the console API is rarely used within mdns component I would tend to accept this PR, WYT @suren-gabrielyan-espressif ?

The ultimate goal should migration of the mdns_console to other console components, cc @espressif-abhikroy

@david-cermak david-cermak requested a review from gabsuren March 18, 2024 15:44
@david-cermak
Copy link
Collaborator

@rretanubun Thanks for your contribution!
@suren-gabrielyan-espressif Thanks for the quick review!

@david-cermak david-cermak merged commit 963b32e into espressif:master Mar 20, 2024
54 checks passed
@rretanubun rretanubun deleted the make-mdns-console-kcconfig branch March 20, 2024 12:35
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.

6 participants