open-dynamic-robot-initiative / master-board

Hardware and Firmware of the Solo Quadruped Master Board
BSD 2-Clause "Simplified" License
120 stars 41 forks source link

Handle out-of-range motor params without wraparound #117

Closed tlbtlbtlb closed 2 years ago

tlbtlbtlb commented 2 years ago

Description

Avoid wraparound when converting floating point values to fixed-point. Previously, a value for Kp (the position feedback term) just above 5.0 would wrap around to 0 when converted to uint16_t. Now it gets clamped at the minimum & maximum representable value. All the control params (Kp, Kd, i_sat, current_ref, velocity_ref, position_ref) are handled similarly.

Also add tests for the above, using the catch2 test harness. make test will run them.

How I Tested

sdk/master_board_sdk/tests/test_protocol.cpp covers many cases

I fulfilled the following requirements

nim65s commented 2 years ago

This looks good for me, thanks ! Could you just remove this last commit ? I think it is not supposed to get in this PR.

Also, I think we should add some CMake for catch2. (i didn't even notice that we had a Makefile here…) ; but I can handle this if you want.

tlbtlbtlb commented 2 years ago

Sorry, I'm still learning how to do PRs properly. The branch this PR references now only contains the changes relevant to FP overflow.

nim65s commented 2 years ago

yes, PR are opened on a particular base branch, and while the PR is open, all interactions with that branch will update the PR.

To deal with this, a common usage is to create a branch specific for a PR in your fork : this way you'll be able to continue other tasks on the main branch.

Anyway this looks OK now, I'll squash all those commits and merge, thanks !

nim65s commented 2 years ago

Well, I should have run the tests before merging… Here are the results on my system:


[0/1] Running tests...
Test project /tmp/master-board/sdk/master_board_sdk/build
1/1 Test #1: sdk_tests1/1 Test #1: sdk_tests.........................***Failed    0.00 sec
Randomness seeded to: 2038158713

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
sdk_tests is a Catch2 v3.1.0 host application.
Run with -? for options

-------------------------------------------------------------------------------
FLOAT_TO_D32QN and friends work
-------------------------------------------------------------------------------
../tests/test_protocol.cpp:5
...............................................................................

../tests/test_protocol.cpp:27: FAILED:
  CHECK( ((int32_t) std::min(std::max(((std::numeric_limits<double>::quiet_NaN()) * (1<<(24))), -2147483647.0), 2147483647.0)) == int32_t(0) )
with expansion:
  -2147483648 == 0

===============================================================================
test cases:  1 |  0 passed | 1 failed
assertions: 28 | 27 passed | 1 failed

0% tests passed, 1 tests failed out of 1

Total Test time (real) =   0.01 sec

The following tests FAILED:
     1 - sdk_tests (Failed)
Errors while running CTest
FAILED: CMakeFiles/test.util 
tlbtlbtlb commented 2 years ago

I put that test in thinking "I wonder what the behavior even is for converting NaNs?" Both x64/Linux and ARM/Apple give 0, so I left the test in. Probably better to remote the test, since it's not worth master-board trying to guarantee any particular behavior in this case.