core: add ng_pktbuf_stats() to core panic

[miri64]

Prints the current status of the packet buffer in case of a panic.

[OlegHahm]

I don't know if there are any arguments against printing (more) stuff in this handler, but I guess in a panic case there are not a lot of things that could get worse. @gebart, @daniel-k, @haukepetersen, objections?

[miri64]

It helped me a lot finding the bugs in #3496 because I got an idea which areas of the packet buffer got corrupted and then inferring from it the point in code where this happened.

[OlegHahm]

This introduces another ~500 Bytes of ROM for some Cortex-M platforms (though, only 4 for the IoT-LAB_M3 (which is weird enough).

Since we had this discussion recently with another issue, maybe you can find a solution with some DEBUG macro or introduce another define just for debugging gnrc?

[jnohlgard]

@OlegHahm did you do a info-buildsizes-diff run? could you post the results?

[OlegHahm]

I did, but cannot access the results right now.

[miri64]

@OlegHahm do you have them now?

[miri64]

Since we had this discussion recently with another issue, maybe you can find a solution with some DEBUG macro or introduce another define just for debugging gnrc?

First of all ng_pktbuf_stats() is only available and only outputs if DEVELHELP is defined, so we are already in "not for production code" territory.

Also, "with some DEBUG macro" is too unspecific for me to understand what you really mean. But I tried to interpret it and here are my thoughts on that:

1. use ENABLE_DEBUG/DEBUG macro mechanism: ng_pktbuf_stats() is a function called from outside the packet buffer and is thought to be used when there is a fail in the packet buffer in DEBUG mode of the calling module (e.g. when ng_pktbuf_add() returns NULL unexpectatly). So setting the ENABLE_DEBUG inside ng_pktbuf_static.c to debug something outside of ng_pktbuf_static.c sounds to me in strong contrast to the current usage of ENABLE_DEBUG/DEBUG
2. Use log and DEBUG level: The same argument basically applies as to 1.
3. introduce a new macro (which was also your second point): I'm not completely against that but I'm also hesitant to that, since I'm not even sure which debugging/development help macros are currently in the code because is documented nowhere central and I have the feeling that we don't even have a united argument what the functionality of which macro is (best example: DEVELHELP). Also what does this mean for the future:Every time a build path introduced by a macro gets to big we make a new one?

[OlegHahm]

@OlegHahm do you have them now?

Here they are:

text    data    bss     dec     BOARD/BINDIRBASE

ERR     ERR     ERR     ERR     airfy-beacon
ERR     ERR     ERR     ERR     master-bin
ERR     ERR     ERR     ERR     3500-bin

484     0       0       484     arduino-due
38940   172     16844   55956   master-bin
39424   172     16844   56440   3500-bin

0       0       0       0       avsextrem
101584  2284    96017   199885  master-bin
101584  2284    96017   199885  3500-bin

484     0       0       484     cc2538dk
38444   168     16768   55380   master-bin
38928   168     16768   55864   3500-bin

ERR     ERR     ERR     ERR     chronos
ERR     ERR     ERR     ERR     master-bin
ERR     ERR     ERR     ERR     3500-bin

484     0       0       484     ek-lm4f120xl
38392   168     16808   55368   master-bin
38876   168     16808   55852   3500-bin

484     0       0       484     f4vi1
39924   172     16780   56876   master-bin
40408   172     16780   57360   3500-bin

484     0       0       484     fox
40264   168     16784   57216   master-bin
40748   168     16784   57700   3500-bin

484     0       0       484     frdm-k64f
38360   1196    18900   58456   master-bin
38844   1196    18900   58940   3500-bin

4       0       0       4       iot-lab_M3
54216   168     19888   74272   master-bin
54220   168     19888   74276   3500-bin

484     0       0       484     mbed_lpc1768
38232   168     16792   55192   master-bin
38716   168     16792   55676   3500-bin

ERR     ERR     ERR     ERR     msb-430
ERR     ERR     ERR     ERR     master-bin
ERR     ERR     ERR     ERR     3500-bin

ERR     ERR     ERR     ERR     msb-430h
ERR     ERR     ERR     ERR     master-bin
ERR     ERR     ERR     ERR     3500-bin

0       0       0       0       msba2
101264  2284    96017   199565  master-bin
101264  2284    96017   199565  3500-bin

484     0       0       484     msbiot
40240   172     16804   57216   master-bin
40724   172     16804   57700   3500-bin

4       0       0       4       mulle
53848   1220    21644   76712   master-bin
53852   1220    21644   76716   3500-bin

0       0       0       0       native
136670  552     115104  252326  master-bin
136670  552     115104  252326  3500-bin

ERR     ERR     ERR     ERR     nrf51dongle
ERR     ERR     ERR     ERR     master-bin
ERR     ERR     ERR     ERR     3500-bin

580     0       0       580     nucleo-f091
42932   168     16792   59892   master-bin
43512   168     16792   60472   3500-bin

484     0       0       484     nucleo-f303
39900   168     16800   56868   master-bin
40384   168     16800   57352   3500-bin

ERR     ERR     ERR     ERR     nucleo-f334
ERR     ERR     ERR     ERR     master-bin
ERR     ERR     ERR     ERR     3500-bin

484     0       0       484     nucleo-l1
39868   168     16784   56820   master-bin
40352   168     16784   57304   3500-bin

484     0       0       484     openmote
38396   168     16768   55332   master-bin
38880   168     16768   55816   3500-bin

8       0       0       8       pba-d-01-kw2x
53736   1196    21484   76416   master-bin
53744   1196    21484   76424   3500-bin

ERR     ERR     ERR     ERR     pca10000
ERR     ERR     ERR     ERR     master-bin
ERR     ERR     ERR     ERR     3500-bin

ERR     ERR     ERR     ERR     pca10005
ERR     ERR     ERR     ERR     master-bin
ERR     ERR     ERR     ERR     3500-bin

0       0       0       0       pttu
101792  2284    96017   200093  master-bin
101792  2284    96017   200093  3500-bin

0       0       0       0       qemu-i386
208516  5564    128544  342624  master-bin
208516  5564    128544  342624  3500-bin

ERR     ERR     ERR     ERR     redbee-econotag
ERR     ERR     ERR     ERR     master-bin
ERR     ERR     ERR     ERR     3500-bin

576     0       0       576     saml21-xpro
40432   168     16872   57472   master-bin
41008   168     16872   58048   3500-bin

4       0       0       4       samr21-xpro
54572   168     19872   74612   master-bin
54576   168     19872   74616   3500-bin

484     0       0       484     spark-core
40320   168     16784   57272   master-bin
40804   168     16784   57756   3500-bin

ERR     ERR     ERR     ERR     stm32f0discovery
ERR     ERR     ERR     ERR     master-bin
ERR     ERR     ERR     ERR     3500-bin

484     0       0       484     stm32f3discovery
39908   168     16800   56876   master-bin
40392   168     16800   57360   3500-bin

484     0       0       484     stm32f4discovery
40104   172     16796   57072   master-bin
40588   172     16796   57556   3500-bin

ERR     ERR     ERR     ERR     telosb
ERR     ERR     ERR     ERR     master-bin
ERR     ERR     ERR     ERR     3500-bin

484     0       0       484     udoo
38940   172     16844   55956   master-bin
39424   172     16844   56440   3500-bin

ERR     ERR     ERR     ERR     wsn430-v1_3b
ERR     ERR     ERR     ERR     master-bin
ERR     ERR     ERR     ERR     3500-bin

ERR     ERR     ERR     ERR     wsn430-v1_4
ERR     ERR     ERR     ERR     master-bin
ERR     ERR     ERR     ERR     3500-bin

ERR     ERR     ERR     ERR     yunjia-nrf51822
ERR     ERR     ERR     ERR     master-bin
ERR     ERR     ERR     ERR     3500-bin

ERR     ERR     ERR     ERR     z1
ERR     ERR     ERR     ERR     master-bin
ERR     ERR     ERR     ERR     3500-bin

So, as I wrote: the difference is between 0 and +580 bytes.

[jnohlgard]

It seems like the ROM size is negligible on the platforms which have an gnrc compatible 802.15.4 transceiver defined by default (pba-d-01-kw2x, mulle, iot-lab_M3, samr21-xpro...).

ng_pktbuf_stats is called from inside ng_sixlowpan.c, there is a mistake in the preprocessor condition protecting the call though.

[miri64]

ng_pktbuf_stats is called from inside ng_sixlowpan.c, there is a mistake in the preprocessor condition protecting the call though.

That's interesting and not very helpful in the same instance ^^

[OlegHahm]

First of all ng_pktbuf_stats() is only available and only outputs if DEVELHELP is defined, so we are already in "not for production code" territory.

Yes, but as discussed recently 500 bytes are IMO too much for DEVELHELP (which is should not be a debugging mode).

Also, "with some DEBUG macro" is too unspecific for me to understand what you really mean. But I tried to interpret it and here are my thoughts on that:

I meant 3.

1. introduce a new macro (which was also your second point): I'm not completely against that but I'm also hesitant to that, since I'm not even sure which debugging/development help macros are currently in the code because is documented nowhere central

I don't what macros apart from DEBUG exist (besides some leftovers in ccn-lite and aodvv2 that are hopefully removed soon).

There is https://github.com/RIOT-OS/RIOT/wiki/Debugging#using-printf (which might need some extension) and there is http://doc.riot-os.org/core_2include_2debug_8h.html

and I have the feeling that we don't even have a united argument what the functionality of which macro is (best example: DEVELHELP).

DEBUG(F) is mostly for debugging during development of specific module. LOG_* is for logging to detect misbehavior and problems in a deployment. DEVELHELP is for additional helper functions and reporting of error messages during development and testing without significant changes to the behavior of the system as such. It might be that we failed to comply to these rules here and there, but we should try to improve in the future.

Also what does this mean for the future:Every time a build path introduced by a macro gets to big we make a new one?

This change introduces 0.5 kB of code for debugging just one feature. Introducing some mechanism (an additional macro or enabling DEBUG on a per module level or similar) to activate this code only while debugging this module/feature, could be one way to come.

[jnohlgard]

See also #3556

[OlegHahm]

It seems like the ROM size is negligible on the platforms which have an gnrc compatible 802.15.4 transceiver defined by default (pba-d-01-kw2x, mulle, iot-lab_M3, samr21-xpro...).

@gebart, on the long run gnrc should be able run also on MSP430 platforms where ROM size is certainly not negligible.

[jnohlgard]

@OlegHahm I agree, there was a mistake in the preprocessor condition in ng_sixlowpan.c which caused this 500 byte ROM penalty to already have been applied to the 6lowpan platforms. #3556 fixes it.

[OlegHahm]

@OlegHahm I agree, there was a mistake in the preprocessor condition in ng_sixlowpan.c which caused this 500 byte ROM penalty to already have been applied to the 6lowpan platforms. #3556 fixes it.

I don't understand With your fix ng_pktbuf_stats() should be only included if compiling 6lowpan with ENABLE_DEBUG set or am I mistaken?

[jnohlgard]

@OlegHahm I agree with you that gnrc should be able to run on constrained devices, and adding 500 bytes for printing a message during a crash is maybe not the best use of the ROM space. It is mostly useful when debugging deep inside the network stack.

There seems to be some confusion about the real purpose of the DEVELHELP macro. #3333 is also affected by this confusion.

What options do we have for these kinds of development tools/helpers? I would like to run with them always enabled while developing, in order to help finding the source of a crash.

[OlegHahm]

I would like to run with them always enabled while developing, in order to help finding the source of a crash.

100% agreed.

This whole discussion seems to be a good point for the next bi-weekly developer meeting. I will add it to the agenda.

[miri64]

After sleeping over it a little and while working lwIP (which has a similar mechanism) I thought that it might be a good idea to introduce some kind of statistics macro/module for GNRC or even the whole of RIOT. Besides functionality like this one it also could introduce things like packet counters or (current?) resource usage. Apart from solving this problem it might prove helpful for future optimizations and even in production for network set-ups.

[jnohlgard]

@authmillenon
A common statistics API would be very useful for LPM testing as well.

[OlegHahm]

See also #1297

[OlegHahm]

@authmillenon, you still want this solution in or are you going to PR something in the direction of #3500 (comment)?

[miri64]

More in the direction of #3500 (comment). So we can close this for now.