pkg/libfixmath: several improvements and 8bit fix#12687
pkg/libfixmath: several improvements and 8bit fix#12687aabadie merged 7 commits intoRIOT-OS:masterfrom
Conversation
aabadie
added
Type: bug
You still blacklisted the package on 8bit platforms. Unless I miss something, this should no longer be needed due to this fix, right? |
Only the unittests part is blacklisted. The rest is working... |
maribu
added
Reviewed: 1-fundamentals
|
OK, on an ATmega328p clocked at 8MHz I get Could you double the timeout for this test in an additional commit to allow it to pass on slow MCUs? |
Sure, can you tell me a suitable value ? I think I don't have an ATmega clocked at 8MHz (well, this can be changed with fuses but I'm lazy and I don't have the time today ;) ). |
|
The ATmegas all have the same IPC. So when it worked for time x on 16MHz, it will work with |
| except Exception as exc: | ||
| errors += 1 | ||
| print('ERROR {}'.format(line)) | ||
| print('ERROR: {}: {}'.format(line, exc)) |
There was a problem hiding this comment.
The commit message states "tests/pkg_libfixmath: fix PEP8 issues". Its unclear to me which PEP8 issue is fixed by this line (comment originally made in #12706 (comment)).
There was a problem hiding this comment.
The issue was: E722 "bare except". So the fix is to use:
except Exception as exc:And make use of it in the error message. In the end, it improves the message: you know it's an error, you get the line and you get the complete exception message.
There was a problem hiding this comment.
I think the commit message should be "tests/pkg_libfixmath: fix flake8 issues"
fjmolinas
left a comment
fjmolinas
left a comment
There was a problem hiding this comment.
The timeout issue is because of improper ranges. Here are the correct ones, this should fix @maribu issue.
But to be honest this test takes way too long on slow cpu, on native it is fast on any other cpu it is ridiculous how long it takes, It should probably change the test range according to the cpu speed or just use a smaller test set if non-native.
9m54.386s on |
Correct. When running at 8MHz (using the internal oscillator) it should be exactly twice as long. But I like the idea of reducing the number of calculations done to speed up the test (at least for non-native) targets. |
|
Ok, I see there are even more problems than the ones already spotted by this PR. I'll change the test as you suggest (reducing the range for non native board + use precise number of printed calculations). |
|
I went ahead and reworked a bit the test application and script. It should be a little faster now on boards that are not native due to a reduced number of iterations for each sub-tests. On native, the number of iterations of each sub-test is still the same. |
aabadie
force-pushed
the
pr/pkg/libfixmath_update
branch
2 times, most recently
from
9b18ceb to
42ac3a8
Compare
|
Made some tests on real hardware: the last version of the test is much faster now. Using I get the following results (only keep the time results): arduino-unoatmeg256rfr2-xprosamr21-xpronucleo-l073rzWith the previous version (of this PR but without the last commit): arduino-unoatmeg256rfr2-xprosamr21-xproarduino-uno and atmega256rfr2-xpro are both clocked at 16MHz but there is a huge difference between the results. My guess is because the baudrates used for stdio are different: 9600 for arduino-uno, 115200 for the atmega256rfr2 and this makes a huge difference in speed since a lot of data is printed to stdout. |
|
@aabadie I think you arduino-uno test results are wrong, or flipped, I got ~1:50 when testing with the latest commit. |
Indeed, results are now at the right place ;) |
|
@aabadie needs rebasing. |
aabadie
force-pushed
the
pr/pkg/libfixmath_update
branch
from
42ac3a8 to
dc37170
Compare
|
rebased |
|
This PR is good for me now @maribu can you confirm it works on your slow hw? |
8bit architectures are not compatible. round function is not provided by the msp430 toolchain.
The implementation of libfixmath is not compatible with architecture smaller than 32bit
Use pkg_ prefix to have a consistent name with other pkg tests applications
Print the range of iterations for each subtests and catch the value in the Python script. The number of iterations is reduced on boards that are not native, this is because this test takes a lot of time on slow platforms
aabadie
force-pushed
the
pr/pkg/libfixmath_update
branch
from
dc37170 to
07f7ac5
Compare
Too much AVR boards merged ;) Fixed! |
maribu
added
the
Reviewed: 3-testing
maribu
left a comment
maribu
left a comment
There was a problem hiding this comment.
ACK. (One suggestion, but feel free to ignore. Feel also free to squash it right in, if you take it.)
| #ifdef BOARD_NATIVE | ||
| fix16_t _min = fix16_from_dbl(-5.0); | ||
| fix16_t _max = fix16_from_dbl(5.0); | ||
| fix16_t _step = fix16_from_dbl(0.25); | ||
| #else | ||
| fix16_t _min = fix16_from_dbl(-2.0); | ||
| fix16_t _max = fix16_from_dbl(2.0); | ||
| fix16_t _step = fix16_from_dbl(0.25); | ||
| #endif |
There was a problem hiding this comment.
The new IS_USED() feature could be used here. (But I'm not insisting.)
const double _range = (IS_USED(BOARD_NATIVE)) ? 5.0 : 2.0;
fix16_t _min = fix16_from_dbl(- _range);
fix16_t _max = fix16_from_dbl(_range);
fix16_t _step = fix16_from_dbl(0.25);There was a problem hiding this comment.
Indeed. But for macros directly related to the target board, I think IS_USED() is not really helpful since the build on Murdock will cover all the cases.
fjmolinas
added
CI: ready for build
|
Thanks @maribu and @fjmolinas for reviewing! |
4 participants
Contribution description
This PR is providing several improvements to the libfixmath package:
FIXMATH_OPTIMIZE_8BITCFLAGS when building for 8bit architectures. This single change fixes the libfixmath test on Atmegatests/libfixmath_*application intests/pkg_libfixmath_*to make it consistent with other pkg test applications. Because of this change, some pep8 issues were raised by the static test which are also fixed by this PR.FEATURES_BLACKLISTat package level for unittests: 8bit and msp430 architectures are blacklisted and this allows to get rid of theBOARD_BLACKLISTvariable in the unittests test applicationTesting procedure
tests/pkg_libfixmath_unittestsapplication:tests/pkg_libfixmath_unittestsbetween master and this PR (fromBUILD_IN_DOCKER=1 make --no-print-directory -C tests/pkg_libfixmath_unittests/ info-boards-supported):this PR
master
diff
Issues/PRs references
Tick the libfixmath item in #12651