gnrc/sixlowpan: add deprecation note for GNRC_SIXLOWPAN_FRAG_RBUF_AGGRESSIVE_OVERRIDE by PeterKietzmann · Pull Request #13129 · RIOT-OS/RIOT

PeterKietzmann · 2020-01-15T14:35:33Z

Contribution description

When migrating configurations to Kconfig in #13123 we found that GNRC_SIXLOWPAN_FRAG_RBUF_AGGRESSIVE_OVERRIDE is a simple on/off switch which should be represented as a boolean. In Kconfig, a symbol is defined when enabled and it is not generated at all when disabled. In the latter case, the value would be set anyway in the config file :

#ifndef GNRC_SIXLOWPAN_FRAG_RBUF_AGGRESSIVE_OVERRIDE
#define GNRC_SIXLOWPAN_FRAG_RBUF_AGGRESSIVE_OVERRIDE    (1)
#endif

Offline we agreed with @jia200x and @leandrolanzieri that inverting the logic of the macro is the cleanest solution to this problem. As the semantics change, we changed the macro name and added a compile time error in case someone tries to use the old macro and we added a deprecation note to the documentation.

Testing procedure

make doc and check if documentation looks alright.
With examples/gnrc_networking send large ping requests between two nodes so that fragmentation takes place, e.g: ping6 -c 10 -i 1000 -s 1000 <IPv6 dst addr> .
Build with CFLAGS=-DGNRC_SIXLOWPAN_FRAG_RBUF_KEEP_ENTRIES=1 and re-run 2. on nodes.

Issues/PRs references

#12888, #13123

miri64 · 2020-01-15T14:55:57Z

* @miri64 is there anything else one should test?

Both possible configurations should be checked, I would say.

However, I don't get why we need to invert the logic here and moreover remove support already for it (the title says "deprecate", but you remove the #define altogether). Can't the Kconfig switch (if defined) determine the value of GNRC_SIXLOWPAN_FRAG_RBUF_AGGRESSIVE_OVERRIDE instead? This way it would work both with legacy code and the Kconfig integration effort.

miri64 · 2020-01-15T14:58:26Z

(and adhere to our established deprecation process)

miri64 · 2020-01-15T15:01:00Z

Something like

/**
 * ...
 */
#ifndef GNRC_SIXLOWPAN_FRAG_RBUF_KEEP_ENTRIES
#define GNRC_SIXLOWPAN_FRAG_RBUF_KEEP_ENTRIES              (0)
#endif

/**
 * @brief ...
 * @deprecated Use inverse `GNRC_SIXLOWPAN_FRAG_RBUF_KEEP_ENTRIES` with instead. 
 *             Will be removed after 2020.10 release.
 */
#ifndef GNRC_SIXLOWPAN_FRAG_RBUF_AGGRESSIVE_OVERRIDE
#ifdef GNRC_SIXLOWPAN_FRAG_RBUF_KEEP_ENTRIES
#define GNRC_SIXLOWPAN_FRAG_RBUF_AGGRESSIVE_OVERRIDE    (!GNRC_SIXLOWPAN_FRAG_RBUF_KEEP_ENTRIES)
#else /* GNRC_SIXLOWPAN_FRAG_RBUF_KEEP_ENTRIES */
#define GNRC_SIXLOWPAN_FRAG_RBUF_AGGRESSIVE_OVERRIDE    (1)
#endif /* GNRC_SIXLOWPAN_FRAG_RBUF_KEEP_ENTRIES */
#endif

miri64 · 2020-01-15T15:03:50Z

Also GNRC_SIXLOWPAN_FRAG_RBUF_KEEP_ENTRIES seems off as a name. It implies, entries are always kept. However, they are still dropped when they time out. I have to think about the name to give a constructive feedback (i.e. an alternative).

PeterKietzmann · 2020-01-15T15:46:29Z

@miri64 like that? I rebuild the doc and executed the above ping command running examples/gnrc_networking on the iotlab testbed with GNRC_SIXLOWPAN_FRAG_RBUF_KEEP_ENTRIES=1 and GNRC_SIXLOWPAN_FRAG_RBUF_KEEP_ENTRIES=0. Looks alright.

miri64 · 2020-01-15T15:55:53Z

Yepp. Still don't like the name of the new macro though (and still don't have a better name :-()

PeterKietzmann · 2020-01-15T16:10:08Z

GNRC_SIXLOWPAN_FRAG_RBUF_DONT_OVERRIDE ?

miri64 · 2020-01-15T16:30:58Z

GNRC_SIXLOWPAN_FRAG_RBUF_DONT_OVERRIDE ?

GNRC_SIXLOWPAN_FRAG_RBUF_DO_NOT_OVERRIDE would be better in that case :-). But in general 👍

PeterKietzmann · 2020-01-15T16:57:17Z

@miri64 thanks for the immediate feedback. I changed the name accordingly and squashed right away.

fjmolinas · 2020-01-15T20:43:32Z

@miri64 is the status OK for you?

sys/include/net/gnrc/sixlowpan/config.h

miri64

@miri64 thanks for the immediate feedback. I changed the name accordingly and squashed right away.

Sorry, I was rushing to the theater at that moment. Here is some more feedback on the doc.

sys/include/net/gnrc/sixlowpan/config.h

miri64 · 2020-01-15T22:28:19Z

sys/include/net/gnrc/sixlowpan/config.h

+ *
+ * When not set, it will cause the reassembly buffer to
+ * override the oldest entry no matter what. When set, only the oldest
+ * entry that is older than @ref GNRC_SIXLOWPAN_FRAG_RBUF_TIMEOUT_US will be


No... "When set, only incomplete entries that are older than ...TIMEOUT_US will be removed from the reassembly buffer." If and only if you want to keep the information that lingering entries are also removed, you can add something like "On creation of a new entry the timeout period of older entries is also checked."

miri64 · 2020-01-15T22:31:02Z

Also, please adapt the PR title for this PR to the current state for future reference.

PeterKietzmann · 2020-01-16T08:30:01Z

@miri64, @leandrolanzieri please look at my latest changes

sys/include/net/gnrc/sixlowpan/config.h

miri64 · 2020-01-16T08:59:25Z

sys/include/net/gnrc/sixlowpan/config.h

 * overwritten (they will still timeout normally if reassembly buffer is not
 * full).
+ *
+ * @deprecated Use inverse `GNRC_SIXLOWPAN_FRAG_RBUF_DO_NOT_OVERRIDE` with instead.


Suggested change

* @deprecated Use inverse `GNRC_SIXLOWPAN_FRAG_RBUF_DO_NOT_OVERRIDE` with instead.

* @deprecated Use inverse of `GNRC_SIXLOWPAN_FRAG_RBUF_DO_NOT_OVERRIDE` instead.

?

Also @ref GNRC_SIXLOWPAN_FRAG_RBUF_DO_NOT_OVERRIDE is safer, as this way Doxygen checks if the variable exists (and also creates a link).

PeterKietzmann · 2020-01-16T10:36:26Z

@leandrolanzieri, @miri64 I addressed your comments. Can I squash?

miri64

For my part you can squash (including the fixes for the comments below).

sys/include/net/gnrc/sixlowpan/config.h

PeterKietzmann · 2020-01-16T10:45:45Z

Applied @miri64's suggested changes and squashed

miri64 · 2020-01-16T10:49:23Z

You can remove the multiple 'Co-Authored-By:' definitions from the commit message. I don't really need the attribution :D (if you insist, please use my FU mail address).

miri64

Other than that: ACK.

leandrolanzieri

Documentation looks good and GNRC_SIXLOWPAN_FRAG_RBUF_AGGRESSIVE_OVERRIDE has the expected values with and without defining GNRC_SIXLOWPAN_FRAG_RBUF_DO_NOT_OVERRIDE. ACK.

PeterKietzmann · 2020-01-16T13:00:42Z

I saw green lights for a short moment now lets see if removing the co-authorship changes something

miri64 · 2020-01-16T14:56:58Z

And go.

PeterKietzmann added Area: network Area: Networking TF: Config Marks issues and PRs related to the work of the Configuration Task Force labels Jan 15, 2020

PeterKietzmann requested review from leandrolanzieri and miri64 January 15, 2020 14:35

PeterKietzmann mentioned this pull request Jan 15, 2020

Kconfig: Expose gnrc/sixlowpan configurations #13123

Merged

PeterKietzmann force-pushed the pr_sixlowpan_aggr_override branch from 9e6026e to f57ced3 Compare January 15, 2020 15:44

PeterKietzmann closed this Jan 15, 2020

PeterKietzmann reopened this Jan 15, 2020

PeterKietzmann force-pushed the pr_sixlowpan_aggr_override branch from f57ced3 to 46325ca Compare January 15, 2020 16:56

PeterKietzmann added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 15, 2020

leandrolanzieri reviewed Jan 15, 2020

View reviewed changes

sys/include/net/gnrc/sixlowpan/config.h Outdated Show resolved Hide resolved

miri64 requested changes Jan 15, 2020

View reviewed changes

PeterKietzmann changed the title ~~gnrc/sixlowpan: deprecate GNRC_SIXLOWPAN_FRAG_RBUF_AGGRESSIVE_OVERRIDE~~ gnrc/sixlowpan: add deprecation note for GNRC_SIXLOWPAN_FRAG_RBUF_AGGRESSIVE_OVERRIDE Jan 16, 2020

leandrolanzieri reviewed Jan 16, 2020

View reviewed changes

sys/include/net/gnrc/sixlowpan/config.h Outdated Show resolved Hide resolved

miri64 reviewed Jan 16, 2020

View reviewed changes

sys/include/net/gnrc/sixlowpan/config.h Outdated Show resolved Hide resolved

sys/include/net/gnrc/sixlowpan/config.h Outdated Show resolved Hide resolved

PeterKietzmann force-pushed the pr_sixlowpan_aggr_override branch from f59d24c to 688c5f6 Compare January 16, 2020 10:44

miri64 approved these changes Jan 16, 2020

View reviewed changes

leandrolanzieri approved these changes Jan 16, 2020

View reviewed changes

gnrc/sixlowpan: deprecate GNRC_SIXLOWPAN_FRAG_RBUF_AGGRESSIVE_OVERRIDE

c7b6dc5

PeterKietzmann force-pushed the pr_sixlowpan_aggr_override branch from 688c5f6 to c7b6dc5 Compare January 16, 2020 12:59

miri64 merged commit 97a71bc into RIOT-OS:master Jan 16, 2020

miri64 mentioned this pull request Jan 21, 2020

gnrc_sixlowpan_frag: initial import of minimal forwarding #11068

Merged

miri64 mentioned this pull request Jan 31, 2020

gnrc_sixlowpan_frag_rb: fix doc on DO_NOT_OVERRIDE #13245

Merged

leandrolanzieri added this to the Release 2020.04 milestone Feb 20, 2020

leandrolanzieri added the Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation label Feb 20, 2020

miri64 mentioned this pull request Oct 28, 2020

gnrc_sixlowpan_rb: remove GNRC_SIXLOWPAN_FRAG_RBUF_AGGRESSIVE_OVERRIDE #15324

Merged

leandrolanzieri mentioned this pull request Jan 18, 2022

drivers/atwinc15x0: add Kconfig #17382

Open

	* @deprecated Use inverse `GNRC_SIXLOWPAN_FRAG_RBUF_DO_NOT_OVERRIDE` with instead.
	* @deprecated Use inverse of `GNRC_SIXLOWPAN_FRAG_RBUF_DO_NOT_OVERRIDE` instead.

Conversation

PeterKietzmann commented Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

miri64 commented Jan 15, 2020

Uh oh!

miri64 commented Jan 15, 2020

Uh oh!

miri64 commented Jan 15, 2020

Uh oh!

miri64 commented Jan 15, 2020

Uh oh!

PeterKietzmann commented Jan 15, 2020

Uh oh!

miri64 commented Jan 15, 2020

Uh oh!

PeterKietzmann commented Jan 15, 2020

Uh oh!

miri64 commented Jan 15, 2020

Uh oh!

PeterKietzmann commented Jan 15, 2020

Uh oh!

fjmolinas commented Jan 15, 2020

Uh oh!

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miri64 Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

miri64 commented Jan 15, 2020

Uh oh!

PeterKietzmann commented Jan 16, 2020

Uh oh!

Uh oh!

miri64 Jan 16, 2020

Choose a reason for hiding this comment

Uh oh!

miri64 Jan 16, 2020

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann commented Jan 16, 2020

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PeterKietzmann commented Jan 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Jan 16, 2020

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri left a comment

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann commented Jan 16, 2020

Uh oh!

miri64 commented Jan 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PeterKietzmann commented Jan 15, 2020 •

edited

Loading

PeterKietzmann commented Jan 16, 2020 •

edited

Loading