Math functions #819

flixr · 2014-09-01T19:03:22Z

Convert quite a few math macros to functions.
This should make the code easier to use, debug and wrap externally. Additionally code size probably reduces since it's not all inline anymore...

Also cleaned up all the headers and added extern "C" to all the header files the math lib can easier be used in C++ applications.

So far the macros all still exist and call the respective functions for backwards compatibility.

headers, extern C, etc...

flixr · 2014-09-01T19:24:43Z

There are still a few more macro that should be changed to functions and more cleanup and doxygen docs (like putting into appropriate groups, etc) would also be nice...

But main further questions are

really keep Mat33 and RMat?
change all the struct Foo { bar baz; }; to typedef struct { bar baz; } Foo; so we don't have to write struct all over the place?
keep all the type specific (e.g. FLOAT_VECT3_x) macros that just forward to the generic (e.g. VECT3_x) versions?
some macros we should simply remove?
change all the rest of the code to use the functions directly? (as e.g. started in flixr@afd8bbc)

esden · 2014-09-01T19:49:16Z

Here my 22c:

struct vs. typedef: I am very much against using typedef struct as this hides the fact that the variable is a struct. It might be easier to write without the struct part (text editor completion is your friend) but for any casual reader as well as developers that did not write the code, who want to make sense of the code, explicit designation is very useful. I know it is for me. (I know google coding style agrees with me here, but I know there is an equally strong camp of people preaching the exact opposite for other reasons)
type specific VECT macros: When I looked into removing the macros not so long ago I was asking myself the same question. The only reason to use separate ones is for readability I guess and consistency. This way all macros were prefixed with FLOAT or FP. I would vote in getting rid of the prefixes here. I think we need to have some code examples to see how the readability changes and if it helps to have the redundant data encoded in the macro names.
change to use only functions: A definitive yes from me as far as that is possible and makes practical sense of course. :)

flixr · 2014-09-10T16:35:26Z

struct vs. typedef personally I really don't mind typedefs, in most cases it is already clear from the typedef name that it is a struct (i.e. Int32Vect3), I mean what else is it supposed to be?
But I don't have a strong opinion on this...
type specifc macros I would vote for getting rid of the prefixed versions where we have a generic macro

@gautierhattenberger @dewagter any comments on these open questions?

dewagter · 2014-09-10T18:31:52Z

no strong opinion on either one, although I lightly prefer the shorter
typedef (which is closer to classes) as long as it is obvious from the name
(can the style script check this too?)

-Christophe

On Wed, Sep 10, 2014 at 6:35 PM, Felix Ruess notifications@github.com
wrote:

struct vs. typedef personally I really don't mind typedefs, in
most cases it is already clear from the typedef name that it is a struct
(i.e. Int32Vect3), I mean what else is it supposed to be? But I don't have
a strong opinion on this...

type specifc macros I would vote for getting rid of the prefixed
versions where we have a generic macro

@gautierhattenberger https://github.com/gautierhattenberger @dewagter
https://github.com/dewagter any comments on these open questions?

—
Reply to this email directly or view it on GitHub
#819 (comment).

flixr · 2014-09-10T18:43:56Z

No, a style checker can not check if you should be using struct or typedef...

find sw -path sw/airborne/math -prune -o -name "*.[ch]" -exec sed -ri 's/FLOAT_(VECT[23]_(ASSIGN|COPY|SMUL|DIFF|SUM|ADD|SUB|DOT_PRODUCT|CROSS_PRODUCT))/\1/g' {} +

flixr · 2014-09-10T19:24:32Z

Some discussion about pure struct vs. typedef: http://stackoverflow.com/questions/1675351/typedef-struct-vs-struct-definitions

gautierhattenberger · 2014-09-11T13:10:20Z

Quoting the coding style from kernel.org:

In general, a pointer, or a struct that has elements that can reasonably be directly accessed should never be a typedef.

I'm in favor of keeping the struct keyword

Concerning macros, I think we can drop the type specific macros, especially when they are already mapped to the generic version

flixr · 2014-09-11T13:35:24Z

OK, let's keep the struct keyword...

- norm functions/macros return the scalar value, don't take norm as arg - remove some type specific macros where the generic ones can be used

for consistency always call the fuctions x_rmat_[transp]_vmult, FLOAT_RMAT_VECT3_MUL and FLOAT_RMAT_VECT3_TRANSP_MUL are removed.

sed to the rescue: find sw -path sw/airborne/math -prune -o -name "*.[ch]" -exec sed -i 's/\([ ]*[A-Z0-9_]*QUAT_OF[A-Z0-9_]*(\)\([a-zA-Z0-9_]*,[ ]*\)\([a-zA-Z0-9_]*)\)/\L\1\E\&\2\&\3/g' {} + and so on...

- use float_quat_vmult instead of FLOAT_QUAT_RMAT_B2N|N2B - use float_quat_derivative instead of float_quat_vmul_left

Convert most type specific macros to functions and general cleanup. This should make the code easier to use, debug and wrap externally. Additionally code size probably reduces since it's not all inline anymore... Also cleaned up all the headers and added extern "C" to all the header files the math lib can easier be used in C++ applications. So far the macros still exist and call the respective functions for backwards compatibility. Merging pull request #819 * math_functions: (32 commits) [math][ins] rewrite/move some math to ins_float_invariant [airborne] use math functions instead of macros [math] some dox updates [math] more cleanup, e.g. rmat_[transp]_vmult [math] rmat|quat_identity functions [math] add double versions for vect3/quat norm [math] VECTx_NORM updates [math] int32_rmat_*mult functions [math] algebra: remove some more type specific macros [math] more x_integrate_fi functions [math] fix code style [math] geodetic: RMat instead of Mat33 for ltp_of_ecef [math] remove some unused or type specifc VECTx macros [math] add float_quat_integrate_fi [math] set default PAPARAZZI_SRC [test] add math c files [math] add missing alias [math] wgs84_ellipsoid_to_geoid as function [math] use math functions in orientation conversion [misc] fix natnet2ivy compilation ...

fvantienen · 2014-11-17T15:49:28Z

This pull requests contains errors in the calculation of the int32_vect2_norm function, which leads to unexpected heading behaviour. And maybe even more problems! But the current code is unsafe to fly at least!
Shouldn't we write test cases for the simple algebra functions? Or shouldn't we at least test in the simulator before merging?

esden · 2014-11-17T20:57:13Z

A really big 👍 to add a unit test suite for the math libraries! We should have had them for a long time. :)

fvantienen · 2014-11-17T22:28:31Z

If someone can make a simple setup for the tests, me and @EwoudSmeur are willing to write the test cases.

esden · 2014-11-18T06:52:30Z

I have started a very basic skeleton using check. It obviously needs more than just half an hour of work, but it is a start. :) https://github.com/esden/paparazzi/tree/math_tests

flixr · 2014-11-19T22:58:53Z

For reference:

the int32_vect2_norm function should be fixed with [math] some cleanup and fixes for fixedpoint functions #956
alternative test skeleton to the one from @esden (using libtap instead of check) proposed in [tests] skeleton for testing math lib with libtap #958

esden · 2014-11-20T00:58:46Z

I have to say I like the way tap is being written. The only question that I did not see an answer to is if libtap is using forking to make sure that the tests are being caught if they lead to a core dump. This is one of the best features of libcheck.

If libtap does the same thing I am all for it.

flixr added 19 commits August 28, 2014 18:54

[math] pprz_algebra_float: convert some macros to functions

ef2d4ed

[math] pprz_algebra_double: convert some macros to functions

4a1f4b1

[math] pprz_algebra_int: convert some macros to functions

4d90636

[math] pprz_algebra_int: convert more macros to functions

1f87b90

[math] convert some trig macros to functions

ce002e7

[math] int32_sqrt as function

27c8ace

[test] update test_algebra

ae12f27

[math] add double_quat_vmult, fixes for nps

a65df6e

[math] cleanup

88d33da

headers, extern C, etc...

[math] algebra_double functions in c file

eb82253

[math] algebra_float functions in c file

0d443e8

[math] algebra_int functions in c file

8cbbf0f

[misc] fix natnet2ivy compilation

976ef3d

[math] use math functions in orientation conversion

2fc981e

[math] wgs84_ellipsoid_to_geoid as function

97edec1

[math] add missing alias

ec3d292

[test] add math c files

fd4454b

[math] set default PAPARAZZI_SRC

561d318

[math] add float_quat_integrate_fi

43fe5cd

[math] remove some unused or type specifc VECTx macros

8d52fef

find sw -path sw/airborne/math -prune -o -name "*.[ch]" -exec sed -ri 's/FLOAT_(VECT[23]_(ASSIGN|COPY|SMUL|DIFF|SUM|ADD|SUB|DOT_PRODUCT|CROSS_PRODUCT))/\1/g' {} +

flixr added 2 commits September 10, 2014 21:39

[math] geodetic: RMat instead of Mat33 for ltp_of_ecef

43d563f

[math] fix code style

63d67a3

[math] more x_integrate_fi functions

aa10dbe

[math] algebra: remove some more type specific macros

1a8be65

flixr added 8 commits September 11, 2014 16:44

[math] int32_rmat_*mult functions

3f98492

[math] VECTx_NORM updates

336defa

- norm functions/macros return the scalar value, don't take norm as arg - remove some type specific macros where the generic ones can be used

[math] add double versions for vect3/quat norm

b8349ce

[math] rmat|quat_identity functions

c7fcf10

[math] more cleanup, e.g. rmat_[transp]_vmult

8d6d843

for consistency always call the fuctions x_rmat_[transp]_vmult, FLOAT_RMAT_VECT3_MUL and FLOAT_RMAT_VECT3_TRANSP_MUL are removed.

[math] some dox updates

a3e860c

[airborne] use math functions instead of macros

56457d0

sed to the rescue: find sw -path sw/airborne/math -prune -o -name "*.[ch]" -exec sed -i 's/\([ ]*[A-Z0-9_]*QUAT_OF[A-Z0-9_]*(\)\([a-zA-Z0-9_]*,[ ]*\)\([a-zA-Z0-9_]*)\)/\L\1\E\&\2\&\3/g' {} + and so on...

[math][ins] rewrite/move some math to ins_float_invariant

dd1746b

- use float_quat_vmult instead of FLOAT_QUAT_RMAT_B2N|N2B - use float_quat_derivative instead of float_quat_vmul_left

flixr merged commit dd1746b into master Sep 15, 2014

flixr deleted the math_functions branch September 15, 2014 15:22

flixr added the Refactoring label Nov 18, 2014

flixr mentioned this pull request Nov 18, 2014

add more tests for math and run them on CI #954

Closed

flixr added this to the v5.4 milestone Nov 24, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Math functions #819

Math functions #819

flixr commented Sep 1, 2014

flixr commented Sep 1, 2014

esden commented Sep 1, 2014

flixr commented Sep 10, 2014

dewagter commented Sep 10, 2014

flixr commented Sep 10, 2014

flixr commented Sep 10, 2014

gautierhattenberger commented Sep 11, 2014

flixr commented Sep 11, 2014

fvantienen commented Nov 17, 2014

esden commented Nov 17, 2014

fvantienen commented Nov 17, 2014

esden commented Nov 18, 2014

flixr commented Nov 19, 2014

esden commented Nov 20, 2014

Math functions #819

Math functions #819

Conversation

flixr commented Sep 1, 2014

flixr commented Sep 1, 2014

esden commented Sep 1, 2014

flixr commented Sep 10, 2014

dewagter commented Sep 10, 2014

flixr commented Sep 10, 2014

flixr commented Sep 10, 2014

gautierhattenberger commented Sep 11, 2014

flixr commented Sep 11, 2014

fvantienen commented Nov 17, 2014

esden commented Nov 17, 2014

fvantienen commented Nov 17, 2014

esden commented Nov 18, 2014

flixr commented Nov 19, 2014

esden commented Nov 20, 2014