Skip to content

Conversation

@kb2ma
Copy link
Member

@kb2ma kb2ma commented Mar 25, 2017

gcoap depends on nanocoap. However, the RIOT pkg for nanocoap includes messaging functionality that gcoap already implements. Specifically, gcoap does not use nanocoap_sock.c nor some functions in the remaining nanocoap.c implementation file. Removal of this functionality results in a savings of 743 B in the text section of an executable. See the Memory Use page in the gcoap wiki.

This PR patches nanocoap to remove the unnecessary file and functions based on the presence of the NANOCOAP_UTIL_ONLY flag in the build environment. The gcoap Make files have been updated to include this flag.

Depends on PR #6469.

@haukepetersen
Copy link
Contributor

Sorry for the late reply. I think the changes make much sense in general, but can we maybe pimp them slightly:

  • apply the changes to nanocoap directly to the nanocoap source (@kaspar would you go with this?). This way we don't need to patch anything regarding this
  • the export NANOCOAP_UTIL_ONLY := 1 could probably be moved from the application makefiles right into the gcaop Makefile?!

@kb2ma
Copy link
Member Author

kb2ma commented May 19, 2017

Thanks for the review, @haukepetersen. I would be happy to rework the addition of the macro into the upstream nanocoap source if @kaspar030 is willing. I thought about this approach originally. However, the definition of what constitutes a "utility" function can be application specific. For example, my definition always includes coap_build_hdr() as a utility function, but conditionally includes coap_build_reply() as a non-utility function. It just seemed less contentious to make that determination in RIOT rather than upstream.

I agree completely about where to define the NANOCOAP_UTIL_ONLY macro. Similar to #7071, this should be defined in a single place.

@haukepetersen
Copy link
Contributor

@kaspar030: as we talked about offline on Friday: could you please look over this PR and state your opinion about the feasibility of factoring out the requested features from nanocoap directly? Thx

@haukepetersen haukepetersen added State: waiting for other PR State: The PR requires another PR to be merged first Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking labels May 23, 2017
@haukepetersen haukepetersen added this to the Release 2017.07 milestone May 23, 2017
@aabadie
Copy link
Contributor

aabadie commented Jul 15, 2017

#6469 is merged. Can you rebase @kb2ma ?

@haukepetersen
Copy link
Contributor

Ok, no reaction from @kaspar030, so I'd say let's move on with this PR in the current form, and maybe we can move things into nanocoap at some point if we are bored and run out of things to do some later point in time... :-)

@kb2ma
Copy link
Member Author

kb2ma commented Jul 17, 2017

Sounds good. I plan to rebase later today.

@haukepetersen
Copy link
Contributor

perfect, thanks!

@kb2ma kb2ma force-pushed the nanocoap/util_only branch from 69b9dd0 to a62288d Compare July 18, 2017 03:57
@kb2ma
Copy link
Member Author

kb2ma commented Jul 18, 2017

Rebased, but @haukepetersen, @aabadie please hold off before further action. I need to review myself first.

@haukepetersen
Copy link
Contributor

easily done :-)

@kb2ma
Copy link
Member Author

kb2ma commented Jul 20, 2017

Closing because this PR is unnecessary due to a blunder on my part. Apologies to the reviewers for the waste of time.

Removal of the nanocoap functions with the macro only affects the size of the compiled object files. It does not affect the size of the executable. The linker simply does not include the unused functions in the executable.

With or without the definition of NANOCOAP_UTIL_ONLY in the make file, the output of the build for samr21-xpro is as shown below. The unused nanocoap functions are not in the executable.

   text	   data	    bss	    dec	    hex	filename
  61528	    364	  16736	  78628	  13324	examples/gcoap/bin/samr21-xpro/gcoap.elf

The difference is in the object files. Output below is from the info-objsize target:

Without NANOCOAP_UTIL_ONLY

   1144	      0	      0	   1144	    478	nanocoap.o (ex /nanocoap.a)
    349	      0	      0	    349	    15d	nanocoap_sock.o (ex /nanocoap.a)

With NANOCOAP_UTIL_ONLY

    708	      0	      0	    708	    2c4	nanocoap.o (ex /nanocoap.a)

nanocoap_sock excluded from build

@kb2ma kb2ma closed this Jul 20, 2017
@kb2ma kb2ma deleted the nanocoap/util_only branch February 5, 2019 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: network Area: Networking State: waiting for other PR State: The PR requires another PR to be merged first Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants