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

[SITL OSX] Fix some of the warnings and add macosx SITL build to workflows #9063

Merged
merged 19 commits into from
May 25, 2023

Conversation

mmosca
Copy link
Collaborator

@mmosca mmosca commented May 21, 2023

clang can be verbose with warnings, but some of it is probably valid when building a 64bit binary.

Highlighted changes:

  • Call float versions of math functions to avoid conversion to double by the compiler (absf, sqrtf, roundf, etc)
  • Make sure floating point constants are marked as floats, to avoid conversion to double by the compiler. (1.0 is a double, 1.0f is a float and when doing math between float and double, all values get upgraded to double!)
  • Changed memcpy_fn in unit test AND SITL builds to be a macro to memcpy (instead of inline function calling memcpy), this fixes linker errors for memcpy as macos compiler mangles the symbol in a different way and would not work with asm("memcpy") call.
  • Some simulator code made heavy use of doubles, but since all the data in INAV is float, that is probably overkill and some functions/macros had float in the name, while upconvertting to double.

Open questions:

  • Should scale in osdFormatCentiNumber be changed to float? It is currently uint32_t but some of the scale defines are, correctly, not integer numbers.
  • I changed CENTIDEGREES_TO_DEGREES to use float on the division path, but the code seems to be ok, or assuming it would be converted to integer anyway. Is this the correct solution?
  • This still does not fix the invalid settings issues, but overall should make it easier to test mac builds.

Mostly promotion from float to double, since OSX is a 64bit build.
Hopefully brew can be used to install packages and we can have a target that is pretty much the same as linux's
@mmosca mmosca changed the title [SITL OSX] Fix some of the warnings in macosx SITL build [SITL OSX] Fix some of the warnings and add macosx SITL build to workflows May 21, 2023
@mmosca mmosca added this to the 7.0 milestone May 22, 2023
@mmosca mmosca marked this pull request as ready for review May 22, 2023 17:24
@mmosca mmosca requested a review from stronnag May 22, 2023 17:24
@mmosca
Copy link
Collaborator Author

mmosca commented May 22, 2023

@Scavanger and @stronnag would you mind having a look and providing some feedback?

@mmosca
Copy link
Collaborator Author

mmosca commented May 22, 2023

A quick indoor line of sight flight test with a quad was successful.

Configurator appears to be happy with the results.

I haven't tested any of the simulators.

@stronnag
Copy link
Collaborator

stronnag commented May 22, 2023

Linux is content with xplane/fl2sitl. I guess FreeBSD will be to (test pending) and probably even Windows (test further pending).

FreeBSD is also happy, suppose I must test it on Windows ... alas

@stronnag
Copy link
Collaborator

And while waiting for the Win10 VM to boot, lets build it on the VisionFive2 board. Yes, it's happy on linux/riscv64 ... looks good to me.

@stronnag
Copy link
Collaborator

And finally, it builds on Windows. Works as well as any other recent patch set ...

[fl2sitm] 19:01:15.232034 Boxflags: 0 Armflags: Overload (0x420)
[fl2sitm] 19:01:15.346264 Boxflags: 0 Armflags: Ready to arm (0x20)
[fl2sitm] 19:01:17.495188 Boxflags: 0 Armflags: Overload (0x420)
[fl2sitm] 19:01:17.714590 Boxflags: 0 Armflags: Ready to arm (0x20)
[fl2sitm] 19:01:17.838657 Stats 10s: Tx: 97 RSSI 0 Stats 96 (0)

@stronnag
Copy link
Collaborator

For completeness, built on an antediluvian MacOS VM

[149/287] Building C object src/main/t...iles/SITL.elf.dir/__/__/flight/imu.c.o
/Users/jrh/Projects/inav/src/main/flight/imu.c:331:55: warning: suggest braces around initialization of subobject [-Wmissing-braces]
    STATIC_FASTRAM fpVector3_t vGyroDriftEstimate = { 0 };                                                     ^

Noting also that the more serious issue that MacOS SITL corrupts its own settings still exists; but that's a different and rather more difficult problem.

@mmosca
Copy link
Collaborator Author

mmosca commented May 22, 2023

For completeness, built on an antediluvian MacOS VM

[149/287] Building C object src/main/t...iles/SITL.elf.dir/__/__/flight/imu.c.o
/Users/jrh/Projects/inav/src/main/flight/imu.c:331:55: warning: suggest braces around initialization of subobject [-Wmissing-braces]
    STATIC_FASTRAM fpVector3_t vGyroDriftEstimate = { 0 };                                                     ^

Noting also that the more serious issue that MacOS SITL corrupts its own settings still exists; but that's a different and rather more difficult problem.

That warning is not on the latest macosx compiler which is used by github (Apple clang version 14.0.3 (clang-1403.0.22.14.1))

I also no longer see the settings corruption over here. Can you try with the binary in the artifacts, instead of building your own?

I may need to change the build a bit to support older versions of MacOSX though, but I only have access to 13 (Ventura)

* Remove redundant .zip from artifact names
@stronnag
Copy link
Collaborator

On my ancient MacOS VM, the settings are always corrupt (even with fresh settings). Fix one corrupt setting, then there's another, fix that, go back one step . The settings ARMing flag can never be cleared. Either with the artefacts build or self built.

But if this doesn't happen on a real, modern Mac (vice my ancient VM), it's not a problem.

@stronnag
Copy link
Collaborator

Thusly