Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eigen 3 Support #1984

Merged
merged 2 commits into from
May 21, 2015
Merged

Eigen 3 Support #1984

merged 2 commits into from
May 21, 2015

Conversation

LorenzMeier
Copy link
Member

@Zefz Still some challenges with the unit tests, but I'm going to keep pounding on it - let me know if you find out something.
I could well imagine that the C++ optimizer (and the resulting 0 us operations) is triggering the assertions, because the actual operation is optimized out. We might need to force the use of the variables (e.g. by printing the vector elements) to resolve this.

@Zefz
Copy link
Contributor

Zefz commented Apr 4, 2015

Eigen has actually it's own test suite to verify correctness of operations (but no performance analysis I think).

Porting code from this test suite to PX4 (such as basicstuff.cpp) might be a better idea than to wrap our own tests which are based on mathlib tests.

Here is the main entry point of the Eigen test suite: https://github.com/PX4/eigen/blob/3.2/test/main.h

@Zefz
Copy link
Contributor

Zefz commented Apr 4, 2015

@LorenzMeier Is there a way to run these tests on my PC without a Pixhawk attached?

@Zefz
Copy link
Contributor

Zefz commented Apr 4, 2015

Seems I was wrong. Eigen3 also has a benchmark to test performance as well as tests to verify correctness: https://github.com/PX4/eigen/tree/3.2/bench

I think we would gain a lot by just using these tests as much as we can instead of writing our own.

@LorenzMeier
Copy link
Member Author

@Zefz Makes a lot of sense. Question is just how much effort we have to sink into it to make that happen vs. fix the current tests.

@davids5
Copy link
Member

davids5 commented Apr 17, 2015

Hi @LorenzMeier @Zefz ,

FYI @NuttX

There was a fix in Nuttx that I want to run by both of you. I think it is innocuous, but I want to check with the experts!

When Eigen was brought in, the file px4_eigen.h was added, and included the definition for

 #define RAND_MAX __RAND_MAX
in gcc-arm-none-eabi-xxxx/arm-none-eabi/include/sys/config.h
#ifdef __frv__
#define __ATTRIBUTE_IMPURE_PTR__ __attribute__((__section__(".sdata")))
#endif
#undef __RAND_MAX
#if __INT_MAX__ == 32767
#define __RAND_MAX 32767
#else
#define __RAND_MAX 0x7fffffff
#endif

I suspect this was needed because NuttX had the definition in stdlib.h as #define MAX_RAND 32767
not #define RAND_MAX 32767

The latest NuttX has fixed this and now defines RAND_MAX as 32767

I believe that we had the actual value as 0x7fffffff by the virtue of the definition and the size of int on the platform.

Is this sufficient or would you like me to submit a patch to Greg?

David

@gregory-nutt
Copy link

This limit is 32768 in the NuttX rand(). Are you using some different rand()? stdlib/lib_rand.c:

int rand(void)
{
return (int)nrand(32768);
}

static unsigned int nrand(unsigned int nLimit)
{
...
/* Loop to be sure a legal random number is generated */

do
{
...
}
while (result >= (unsigned long)nLimit);

return (unsigned int)result;
}

I don't think that the value 0x7fffffff applies in any case.

@dagar
Copy link
Member

dagar commented Apr 20, 2015

I'm looking at running the actual eigen test suite, but it's huge. Just compiling the tests would noticeably increase firmware build times. Should we just pull the entire thing in or define a subset we actually care about?

@LorenzMeier
Copy link
Member Author

We run these tests anyway only in the px4fmu-v2_test config, which has quite some space left.

@LorenzMeier LorenzMeier reopened this Apr 20, 2015
@Zefz
Copy link
Contributor

Zefz commented Apr 20, 2015

I have been experimenting with Eigen on PX4 and noticed a bug in GCC 4.7 when compiling certain Eigen features with -Os (optimize for size). Seems the compiler optimizes away some required templates which makes the linker fail (any other optimzation level such as -O3 solves this issue). It is something to keep in mind for the future. (and these tests!)

@dagar
Copy link
Member

dagar commented May 21, 2015

I attempted to get a single standalone test extracted from the eigen test suite running on a pixhawk, but hit many incompatibilities around libstdc++. The test suite is completely dependant on cmake for both building and running, so it would require some hacking of the eigen test harness, and possibly tests.

@dagar
Copy link
Member

dagar commented May 21, 2015

I did some minor cleanup and now only the quaternion tests are failing (temporarily disabled). I built and ran it across all optimization levels (O0, O1, O2, Os, O3) and saw the same results. Only difference is the operation is optimized out of the TEST_OP loop >= -O2. I didn't hit any asserts or see evidence of templates being optimized away.

@LorenzMeier, @Zefz are you still able to reproduce the problem?

-disable quaternion tests for now
@LorenzMeier
Copy link
Member Author

@dagar If the test op is optimised out, is the test still run, or do you imply that we have to run with -O1? How do the quaternion tests fail?

LorenzMeier added a commit that referenced this pull request May 21, 2015
@LorenzMeier LorenzMeier merged commit 9e5978b into master May 21, 2015
@LorenzMeier LorenzMeier deleted the eigen3 branch May 21, 2015 07:12
@dagar
Copy link
Member

dagar commented May 21, 2015

There are 3 macros
VERIFY_OP - run operation and verify
TEST_OP - run operation 60000 iterations and print average time. This is where the operation is being optimized out of the loop
TEST_OP_VERIFY - both

We should have more tests actually verifying results (VERIFY_OP). It might not be that hard to drop gtest into the tests systemcmd.

Before my last commit the test was failing and crashing at the nonsymmetric matrix test, so the quaternion tests weren't running. They're now failing at warnx("Quaternion method 'from_dcm' or 'toRotationMatrix' outside tolerance!")
I'll see if I can get them working.

@LorenzMeier
Copy link
Member Author

Ok, thanks for the update, that sounds good.

PX4BuildBot added a commit that referenced this pull request May 11, 2023
    - mavlink in PX4/Firmware (153a90e): mavlink/mavlink@3ee5382
    - mavlink current upstream: mavlink/mavlink@18955a0
    - Changes: mavlink/mavlink@3ee5382...18955a0

    18955a04 2023-05-11 Nick Exton - common.xml: Add MAV_RESULT_COMMAND_LONG_ONLY and MAV_RESULT_COMMAND_INT_ONLY to MAV_RESULT (#1982)
b92321ba 2023-03-30 Hamish Willee - Update message_definitions/v1.0/common.xml
58ff70f4 2023-03-30 Hamish Willee - COMMAND_ACK progress/result_param2 clarification
31b4aebb 2023-05-11 Julian Oes - cmake: locally install pip dependencies (#1984)
e9bf6a61 2023-05-10 Hamish Willee - Gimbal manager messages - remove WIP tagging (#1980)
0416967f 2023-05-10 Beat Küng - development: changes to standard flight modes (#1915)
ce00667f 2023-05-04 Jonas Vautherin - New cmakelists (#1977)
fdef5cc0 2023-05-03 Nick Exton - common.xml: Prefer COMMAND_INT when command includes altitude field (#1983)
89676d1d 2023-05-03 Nick Exton - common.xml: Add ZOOM_TYPE_HORIZONTAL_FOV to CAMERA_ZOOM_TYPE enum (#1979)
PX4BuildBot added a commit that referenced this pull request May 17, 2023
    - mavlink in PX4/Firmware (6f9be9f): mavlink/mavlink@3ee5382
    - mavlink current upstream: mavlink/mavlink@18955a0
    - Changes: mavlink/mavlink@3ee5382...18955a0

    18955a04 2023-05-11 Nick Exton - common.xml: Add MAV_RESULT_COMMAND_LONG_ONLY and MAV_RESULT_COMMAND_INT_ONLY to MAV_RESULT (#1982)
b92321ba 2023-03-30 Hamish Willee - Update message_definitions/v1.0/common.xml
58ff70f4 2023-03-30 Hamish Willee - COMMAND_ACK progress/result_param2 clarification
31b4aebb 2023-05-11 Julian Oes - cmake: locally install pip dependencies (#1984)
e9bf6a61 2023-05-10 Hamish Willee - Gimbal manager messages - remove WIP tagging (#1980)
0416967f 2023-05-10 Beat Küng - development: changes to standard flight modes (#1915)
ce00667f 2023-05-04 Jonas Vautherin - New cmakelists (#1977)
fdef5cc0 2023-05-03 Nick Exton - common.xml: Prefer COMMAND_INT when command includes altitude field (#1983)
89676d1d 2023-05-03 Nick Exton - common.xml: Add ZOOM_TYPE_HORIZONTAL_FOV to CAMERA_ZOOM_TYPE enum (#1979)
PX4BuildBot added a commit that referenced this pull request May 18, 2023
    - mavlink in PX4/Firmware (b78e6e8): mavlink/mavlink@3ee5382
    - mavlink current upstream: mavlink/mavlink@18955a0
    - Changes: mavlink/mavlink@3ee5382...18955a0

    18955a04 2023-05-11 Nick Exton - common.xml: Add MAV_RESULT_COMMAND_LONG_ONLY and MAV_RESULT_COMMAND_INT_ONLY to MAV_RESULT (#1982)
b92321ba 2023-03-30 Hamish Willee - Update message_definitions/v1.0/common.xml
58ff70f4 2023-03-30 Hamish Willee - COMMAND_ACK progress/result_param2 clarification
31b4aebb 2023-05-11 Julian Oes - cmake: locally install pip dependencies (#1984)
e9bf6a61 2023-05-10 Hamish Willee - Gimbal manager messages - remove WIP tagging (#1980)
0416967f 2023-05-10 Beat Küng - development: changes to standard flight modes (#1915)
ce00667f 2023-05-04 Jonas Vautherin - New cmakelists (#1977)
fdef5cc0 2023-05-03 Nick Exton - common.xml: Prefer COMMAND_INT when command includes altitude field (#1983)
89676d1d 2023-05-03 Nick Exton - common.xml: Add ZOOM_TYPE_HORIZONTAL_FOV to CAMERA_ZOOM_TYPE enum (#1979)
PX4BuildBot added a commit that referenced this pull request May 18, 2023
    - mavlink in PX4/Firmware (1608d31): mavlink/mavlink@3ee5382
    - mavlink current upstream: mavlink/mavlink@18955a0
    - Changes: mavlink/mavlink@3ee5382...18955a0

    18955a04 2023-05-11 Nick Exton - common.xml: Add MAV_RESULT_COMMAND_LONG_ONLY and MAV_RESULT_COMMAND_INT_ONLY to MAV_RESULT (#1982)
b92321ba 2023-03-30 Hamish Willee - Update message_definitions/v1.0/common.xml
58ff70f4 2023-03-30 Hamish Willee - COMMAND_ACK progress/result_param2 clarification
31b4aebb 2023-05-11 Julian Oes - cmake: locally install pip dependencies (#1984)
e9bf6a61 2023-05-10 Hamish Willee - Gimbal manager messages - remove WIP tagging (#1980)
0416967f 2023-05-10 Beat Küng - development: changes to standard flight modes (#1915)
ce00667f 2023-05-04 Jonas Vautherin - New cmakelists (#1977)
fdef5cc0 2023-05-03 Nick Exton - common.xml: Prefer COMMAND_INT when command includes altitude field (#1983)
89676d1d 2023-05-03 Nick Exton - common.xml: Add ZOOM_TYPE_HORIZONTAL_FOV to CAMERA_ZOOM_TYPE enum (#1979)
PX4BuildBot added a commit that referenced this pull request May 19, 2023
    - mavlink in PX4/Firmware (0ff9816): mavlink/mavlink@3ee5382
    - mavlink current upstream: mavlink/mavlink@18955a0
    - Changes: mavlink/mavlink@3ee5382...18955a0

    18955a04 2023-05-11 Nick Exton - common.xml: Add MAV_RESULT_COMMAND_LONG_ONLY and MAV_RESULT_COMMAND_INT_ONLY to MAV_RESULT (#1982)
b92321ba 2023-03-30 Hamish Willee - Update message_definitions/v1.0/common.xml
58ff70f4 2023-03-30 Hamish Willee - COMMAND_ACK progress/result_param2 clarification
31b4aebb 2023-05-11 Julian Oes - cmake: locally install pip dependencies (#1984)
e9bf6a61 2023-05-10 Hamish Willee - Gimbal manager messages - remove WIP tagging (#1980)
0416967f 2023-05-10 Beat Küng - development: changes to standard flight modes (#1915)
ce00667f 2023-05-04 Jonas Vautherin - New cmakelists (#1977)
fdef5cc0 2023-05-03 Nick Exton - common.xml: Prefer COMMAND_INT when command includes altitude field (#1983)
89676d1d 2023-05-03 Nick Exton - common.xml: Add ZOOM_TYPE_HORIZONTAL_FOV to CAMERA_ZOOM_TYPE enum (#1979)
dagar pushed a commit that referenced this pull request May 19, 2023
    - mavlink in PX4/Firmware (0ff9816): mavlink/mavlink@3ee5382
    - mavlink current upstream: mavlink/mavlink@18955a0
    - Changes: mavlink/mavlink@3ee5382...18955a0

    18955a04 2023-05-11 Nick Exton - common.xml: Add MAV_RESULT_COMMAND_LONG_ONLY and MAV_RESULT_COMMAND_INT_ONLY to MAV_RESULT (#1982)
b92321ba 2023-03-30 Hamish Willee - Update message_definitions/v1.0/common.xml
58ff70f4 2023-03-30 Hamish Willee - COMMAND_ACK progress/result_param2 clarification
31b4aebb 2023-05-11 Julian Oes - cmake: locally install pip dependencies (#1984)
e9bf6a61 2023-05-10 Hamish Willee - Gimbal manager messages - remove WIP tagging (#1980)
0416967f 2023-05-10 Beat Küng - development: changes to standard flight modes (#1915)
ce00667f 2023-05-04 Jonas Vautherin - New cmakelists (#1977)
fdef5cc0 2023-05-03 Nick Exton - common.xml: Prefer COMMAND_INT when command includes altitude field (#1983)
89676d1d 2023-05-03 Nick Exton - common.xml: Add ZOOM_TYPE_HORIZONTAL_FOV to CAMERA_ZOOM_TYPE enum (#1979)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants