Skip to content

tests/xtimer_now32_overflow: simplify#13853

Merged
fjmolinas merged 1 commit intoRIOT-OS:masterfrom
kaspar030:simplify_xtimer_now32_overflow_test
Apr 16, 2020
Merged

tests/xtimer_now32_overflow: simplify#13853
fjmolinas merged 1 commit intoRIOT-OS:masterfrom
kaspar030:simplify_xtimer_now32_overflow_test

Conversation

@kaspar030
Copy link
Contributor

Contribution description

Vastly simplifies and speeds up the regression test added in #13850.

Testing procedure

Test should succeed on master. To see that it would fail, revert 212fe15.

Issues/PRs references

#13850

@kaspar030 kaspar030 added Area: tests Area: tests and testing framework Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Apr 10, 2020
@kaspar030 kaspar030 force-pushed the simplify_xtimer_now32_overflow_test branch from 1e18d31 to f0ca794 Compare April 11, 2020 19:37
@kaspar030
Copy link
Contributor Author

ping @miri64 @JulianHolzwarth

@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 12, 2020
@fjmolinas
Copy link
Contributor

Testing:

  • pr:
RIOT native interrupts/signals initialized.
LED_RED_OFF
LED_GREEN_ON
RIOT native board initialized.
RIOT native hardware initialization complete.

Help: Press s to start test, r to print it is ready
READY
s
START
main(): This is RIOT! (Version: 2020.07-devel-5-gf0ca79-pr-13853)
[SUCCESS]
  • reverted case:
RIOT native interrupts/signals initialized.
LED_RED_OFF
LED_GREEN_ON
RIOT native board initialized.
RIOT native hardware initialization complete.

Help: Press s to start test, r to print it is ready
READY
s
START
main(): This is RIOT! (Version: 2020.07-devel-6-g88a790-pr-13853)
[FAILED]

@fjmolinas fjmolinas added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Apr 14, 2020
@fjmolinas
Copy link
Contributor

The test looks good, and is a simpler test for the regression pointed out in the test README. This test doesn't check the timing of the offsets, but I do not see why that is needed for the regression test intended here, it seems more like a test for xtimer_set_msg64(), tests for the 64bit case seem to generally be missing, but I'm under the impression that the only differ from the 32bit version on the overflow that is tested here:

void _xtimer_set_msg(xtimer_t *timer, uint32_t offset, msg_t *msg, kernel_pid_t target_pid)
{
    _setup_msg(timer, msg, target_pid);
    _xtimer_set64(timer, offset, 0);
}

void _xtimer_set_msg64(xtimer_t *timer, uint64_t offset, msg_t *msg, kernel_pid_t target_pid)
{
    _setup_msg(timer, msg, target_pid);
    _xtimer_set64(timer, offset, offset >> 32);
}

Anyway since I'm not that acquainted with xtimer code, maybe @JulianHolzwarth can chime in and mention if the new test falls short somehow/

@kaspar030
Copy link
Contributor Author

@miri64 @JulianHolzwarth could you take a closer look?

The test that's currently in master is actually buggy:

child.expect(r"now=(\d+)")
offset = int(child.match.group(1))
# get largest possible timeout for expects below
timeout = _get_largest_timeout_difference(timers.values()) / (10**6) + 1
for i in range(timers_numof):
child.expect(r"#(\d):now=(\d+)", timeout=timeout)

(The \d+ are not terminated, leading to partial matches).

@kaspar030 kaspar030 added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Apr 16, 2020
@fjmolinas
Copy link
Contributor

Please squash @kaspar030 !

@kaspar030 kaspar030 force-pushed the simplify_xtimer_now32_overflow_test branch from 809dadb to 4ac5e5b Compare April 16, 2020 13:28
@kaspar030
Copy link
Contributor Author

  • squashed

Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change seems OK to me and the regression is still tested in a simpler way.

@fjmolinas fjmolinas merged commit 85cbc04 into RIOT-OS:master Apr 16, 2020
@leandrolanzieri leandrolanzieri added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Apr 16, 2020
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Apr 16, 2020
@leandrolanzieri
Copy link
Contributor

Backport provided in #13905

@fjmolinas
Copy link
Contributor

I have some asserts with this PR depending on the platform:

BOARD=iotlab-m3 CFLAGS=-DDEBUG_ASSERT_VERBOSE make -C tests/xtimer_now32_overflow/ flash term -j3

START
main(): This is RIOT! (Version: 2020.07-devel-5-g4ac5e-HEAD)
[SUCCESS]
sys/xtimer/xtimer_core.c:278 => 0x8000d49
*** RIOT kernel panic:
FAILED ASSERTION.

*** halted.

iotlab-m3, openbmote-b, usb-kw41z. No issues with samr21-pxro, nrf52dk, nucleo-l14

Seems I should have tested this PR on more platforms, any ideas?

@kaspar030 kaspar030 deleted the simplify_xtimer_now32_overflow_test branch September 9, 2020 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Reviewed: 3-testing The PR was tested according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants