Bugfix in ringbufindex #2046

yatch · 2017-01-11T22:00:51Z

There are two bugs in ringbufindex. They are similar.

While ringbufindex_peek_put() is supposed to return the index of the next element to be added, it returns the next index of the adding element. ringbufindex_peek_get() returns the next index of the first element.

Travis got green on my account. However, 19-z1-rpl-tsch.csc.flaky failed, that is one of non-regression tests of TSCH. Since this one fails even on the tip of the master branch, I would say, this PR doesn't cause the failure...

A regression test for ringbufindex has been introduced under regression-tests/03-base as 04-ringbufindex. Without the fixes, two of them fails.

Random seed: 1
549000 node-1 Rime started with address 0.1.0.1.0.1.0.1
549000 node-1 MAC 00:01:00:01:00:01:00:01 sicslowpan/CSMA/nullrdc, channel check rate 1000 Hz
549000 node-1 Tentative link-local IPv6 address fe80:0000:0000:0000:0201:0001:0001:0001
549000 node-1 Tentative global IPv6 address fd00:0000:0000:0000:0201:0001:0001:0001
549000 node-1 Starting 'ringbuf-index.c test'
549000 node-1 =check-me= SUCEEDED - Init
549000 node-1 =check-me= SUCEEDED - Put
549000 node-1 =check-me= FAILED   - PeekPut: exit at L104
549000 node-1 =check-me= SUCEEDED - Get
549000 node-1 =check-me= FAILED   - PeekGet: exit at L168
549000 node-1 =check-me= SUCEEDED - Size
549000 node-1 =check-me= SUCEEDED - Elements
549000 node-1 =check-me= SUCEEDED - Full
549000 node-1 =check-me= SUCEEDED - Empty
TEST FAILED
Test ended at simulation time: 10149000
 FAIL ಠ_ಠ

All the tests pass on this branch.

$ make 04-ringbufindex.testlog
Running test 04-ringbufindex with random Seed 1: ..... OK

$ cat 04-ringbufindex.testlog
Random seed: 1
549000 node-1 Rime started with address 0.1.0.1.0.1.0.1
549000 node-1 MAC 00:01:00:01:00:01:00:01 sicslowpan/CSMA/nullrdc, channel check rate 1000 Hz
549000 node-1 Tentative link-local IPv6 address fe80:0000:0000:0000:0201:0001:0001:0001
549000 node-1 Tentative global IPv6 address fd00:0000:0000:0000:0201:0001:0001:0001
549000 node-1 Starting 'ringbuf-index.c test'
549000 node-1 Run unit-test
549000 node-1 ---
549000 node-1 =check-me= SUCEEDED - Init
549000 node-1 =check-me= SUCEEDED - Put
549000 node-1 =check-me= SUCEEDED - PeekPut
549000 node-1 =check-me= SUCEEDED - Get
549000 node-1 =check-me= SUCEEDED - PeekGet
549000 node-1 =check-me= SUCEEDED - Size
549000 node-1 =check-me= SUCEEDED - Elements
549000 node-1 =check-me= SUCEEDED - Full
549000 node-1 =check-me= SUCEEDED - Empty
549000 node-1 =check-me= DONE
TEST OK
Test ended at simulation time: 549000

neophob · 2017-01-13T08:52:42Z

Great catch. I saw that the JN platform uses the ringbufindex code as well, what's the impact there?

yatch · 2017-01-13T14:50:21Z

@neophob I don't think this change has any impact on platform/jn516x/dev/micromac-radio.c.

To my understanding, you don't have any problem with this bug as long as you don't use an index returned by ringbufindex_get(). The JN platform gets an index with ringbufindex_peek_get() and uses ringbufindex_get() just to remove the first element in the ringbuf. In fact, ringbufindex_get() removes a different element from one returned by ringbufindex_peek_get(). But, still, consistency is maintained.

simonduq · 2017-01-13T14:58:31Z

Right, if I'm correct it looks like everything was off-by-one, causing no problem (jn nor tsch), but that is definitely unwanted indeed :p
How does 19-z1-rpl-tsch.csc.flaky fail? It should randomly fail to link. But when it links, it normally runs successfully (the run is not flaky).

yatch · 2017-01-16T23:04:51Z

@simonduq Regarding 19-z1-rpl-tsch.csc.flaky, the number of "routing links" on node-1 doesn't get to 8 within the simulation time. I think I've found a bug, which is not in ringbufindex. I'll send a PR later.

simonduq · 2017-01-17T07:38:39Z

Right, looking forward to the PR then :) I guess the bug simply slipped in now with the test disabled :/

yatch · 2017-01-18T20:34:05Z

@simonduq I've created a new PR, #2079. My conclusion is, 19-z1-rpl-tsch.csc.flaky fails because some RPL nodes fail to receive broadcast frames including DIOs. This is caused by a bug in the frame filtering feature of the simulated CC2420 device.

At first, I noticed that the test failed on the commit of bfef0b5, although it succeeded before that. So, I suspected the adaptive scheduling, but I was wrong. I guess, some timing change by bfef0b5 triggers the test failure by chance.

A TSCH node is initialized with frame filtering disabled. As long as the frame filtering is disabled, the node doesn't suffer from the bug of CC2420. The filter is enabled once it sends a unicast (ACK-required) frame. Therefore, if it receives a broadcasted DIO before sending any unicast frame, it can join a DODAG. Otherwise, it cannot not. This happens in a failed test of 19-z1-rpl-tsch.csc.flaky. I'm confused why the frame filtering needs to be turned on and off in TSCH, though.

Anyway, the failure of the test is nothing to do with ringbufindex.

simonduq · 2017-01-18T20:44:26Z

Could it be the recent changes to MAC header that result in frames that the emulated cc2420 filters out?
Maybe it would be wiser to unset TSCH_HW_FRAME_FILTERING by default. It's already manually disabled on the cc2538 platforms in some project-conf.h files.

yatch · 2017-01-18T21:59:23Z

@simonduq In the first place, why does TSCH needs the frame filtering disabled during scanning and waiting for an ACK...? (it's totally off-topic, by the way... 😅)

simonduq · 2017-01-19T11:07:00Z

On some platforms (at least cc2420, which was our initial development platform), the radio would filter out enhanced ACKs, probably due to the len different from 3.

Scanning should work both with and without filtering.

yatch · 2017-01-19T12:47:37Z

@simonduq Thank you for the information! Now I understand what the following comment means, that is found in tsch-slot-operation.c.

/* Entering promiscuous mode so that the radio accepts the enhanced ACK */

greg-king5 · 2017-01-23T11:10:48Z

regression-tests/03-base/code/test-ringbufindex.c

+  if(utp->result == unit_test_failure) {
+    printf("FAILED   - %s: exit at L%u\n", utp->descr, utp->exit_line);
+  } else {
+    printf("SUCEEDED - %s\n", utp->descr);


Typo: SUCEEDED -> SUCCEEDED

Thanks! I've fixed this typo 😄

simonduq · 2017-03-08T09:19:58Z

Good. I've used the fix in multiple testbeds, successfully.
Could you enable in Travis the test added in #2108 ?

yatch · 2017-03-09T02:19:08Z

@simonduq Sure, I can do that today :-)

yatch · 2017-03-09T11:52:46Z

Added one commit enabling TSCH regression testing. Travis ran the test and got green.

simonduq · 2017-03-09T12:06:21Z

👍

simonduq · 2017-03-09T12:06:27Z

Thanks!

cortex-m: do not add LD_*_GROUP by default

greg-king5 reviewed Jan 23, 2017

View reviewed changes

yatch force-pushed the pr/ringbufindex-bugfix branch from 501d85b to be1a14c Compare January 23, 2017 20:20

This was referenced Feb 10, 2017

TSCH: POC of a bug in tsch_queue_flush_nbr_queue() #2108

Merged

TSCH queue add error #2105

Closed

TSCH: TSCH queue not fully flushed/reset after disassociation #1766

Open

yatch added 4 commits March 9, 2017 16:47

Add a regression test for ringbufindex APIs

8df0f7d

ringbufindex: fix a typo in a comment

baaa3da

ringbufindex: explain return values by the APIs

fbf9bb9

ringbufindex: fix bugs of ringbufindex_peek_{put,get}

2d42b91

yatch force-pushed the pr/ringbufindex-bugfix branch from be1a14c to 9acf177 Compare March 9, 2017 07:47

Enable TSCH regression testing

9acf177

simonduq merged commit a24ac86 into contiki-os:master Mar 9, 2017

yatch deleted the pr/ringbufindex-bugfix branch March 9, 2017 12:07

alexrayne pushed a commit to alexrayne/contiki that referenced this pull request Oct 13, 2022

Merge pull request contiki-os#2046 from pjonsson/makefile-narrow-ldgroup

f78dd2d

cortex-m: do not add LD_*_GROUP by default

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix in ringbufindex #2046

Bugfix in ringbufindex #2046

yatch commented Jan 11, 2017

neophob commented Jan 13, 2017

yatch commented Jan 13, 2017

simonduq commented Jan 13, 2017

yatch commented Jan 16, 2017

simonduq commented Jan 17, 2017

yatch commented Jan 18, 2017

simonduq commented Jan 18, 2017

yatch commented Jan 18, 2017

simonduq commented Jan 19, 2017

yatch commented Jan 19, 2017

greg-king5 Jan 23, 2017

yatch Jan 23, 2017

simonduq commented Mar 8, 2017

yatch commented Mar 9, 2017

yatch commented Mar 9, 2017

simonduq commented Mar 9, 2017

simonduq commented Mar 9, 2017

Bugfix in ringbufindex #2046

Bugfix in ringbufindex #2046

Conversation

yatch commented Jan 11, 2017

neophob commented Jan 13, 2017

yatch commented Jan 13, 2017

simonduq commented Jan 13, 2017

yatch commented Jan 16, 2017

simonduq commented Jan 17, 2017

yatch commented Jan 18, 2017

simonduq commented Jan 18, 2017

yatch commented Jan 18, 2017

simonduq commented Jan 19, 2017

yatch commented Jan 19, 2017

greg-king5 Jan 23, 2017

Choose a reason for hiding this comment

yatch Jan 23, 2017

Choose a reason for hiding this comment

simonduq commented Mar 8, 2017

yatch commented Mar 9, 2017

yatch commented Mar 9, 2017

simonduq commented Mar 9, 2017

simonduq commented Mar 9, 2017