Correct platform defaults

[krichardsson]

The autoconfig.h was not included in platform_defaults.h which cased no platform specific defines to not being included.

[matejkarasek]

Including this indeed helps with my issue - defining the PID gains in the platform_defualts.h instead of pid.h 👍

But I feel the behavior is still a bit strange...

E.g. this define already worked even without autoconf.h included...
#define DEFAULT_BAT_LOW_VOLTAGE 6.4f

In some cases, it would be nice to have a default value in the code, but to be able to override that e.g. in platform_defaults_[specificPlatform].h

E.g. for IMU angles this works well with
#ifndef IMU_PHI
#define IMU_PHI 0.0
#endif
https://github.com/flapper-drones/crazyflie-firmware/blob/68df724ac20c1a0f15956f42356ccac4fa652188/src/hal/src/sensors_bmi088_bmp388.c#L151

And IMU_PHI defined also in platform_defaults_[specificPlatform].h

But using the same structure for PID gains in pid.h (e.g. PID_ROLL_RATE_KP) gives redefine error...

[krichardsson]

E.g. this define already worked even without autoconf.h included...
#define DEFAULT_BAT_LOW_VOLTAGE 6.4f

I¨m not sure why (or where) it worked.

In some cases, it would be nice to have a default value in the code, but to be able to override that e.g. in platform_defaults_[specificPlatform].h

You could have the default values defined in platforms_default.h and the overrides in one or more of the platform_defaults_[specificPlatform].h files. Like this:

In platform_defaults_myplatform.h

#define MY_SPECIAL_PARAM 17

and in platform_defaults.h

#pragma once

#include "autoconf.h"

#define __INCLUDED_FROM_PLATFORM_DEFAULTS__

#ifdef CONFIG_PLATFORM_CF2
    #include "platform_defaults_cf2.h"
#endif
#ifdef CONFIG_PLATFORM_BOLT
    #include "platform_defaults_bolt.h"
#endif
#ifdef CONFIG_PLATFORM_TAG
    #include "platform_defaults_tag.h"
#endif

#ifndef MY_SPECIAL_PARAM
    #define MY_SPECIAL_PARAM 4711
#endif

I'm not sure it is that much clearer though, I guess it has to be decided from case to case.

But using the same structure for PID gains in pid.h (e.g. PID_ROLL_RATE_KP) gives redefine error...

Remove the define from pid.h and move it to the platform defaults. The question might be if it should only be in the platform_defaults_[platform].h or also a default in platform_defaults.h. My thinking (in this case) is that there should not be any values in the platform_defaults.h file and only in the platform specific ones. The reason is that the default PID values will be different for pretty much all platforms anyway and it is better that the build fails if a value is not defined for a particular platform.

My thinking when it comes to the values in the platform defaults module, is that they should be safe rather than optimal. Let's use the Bolt as an example. The most important thing is that if someone flashes a Bolt and tries to fly it, is to make it reasonably safe. Optimal settings must be defined based on the configuration anyway and should (in my opinion) be set using persistent parameters. If values are pretty optimal as well, even better :-)

It is possible that we should add functionality to write a set of parameters from file to persistent memory to make it easy to configure a swarm for instance, but that is another story.

[evoggy]

Looks good!

[matejkarasek]

E.g. this define already worked even without autoconf.h included...
#define DEFAULT_BAT_LOW_VOLTAGE 6.4f

I¨m not sure why (or where) it worked.

This is defined only in platform_defaults_[platform].h, for all the platforms individually, and this compiles well even without the autoconf.h included...

In some cases, it would be nice to have a default value in the code, but to be able to override that e.g. in platform_defaults_[specificPlatform].h

You could have the default values defined in platforms_default.h and the overrides in one or more of the platform_defaults_[specificPlatform].h files.

Yes, this works well.

I am Just wondering, why keeping the #ifndef, #define structure in the code sometimes works (e.g. IMU angles) and sometimes not (PID gains).
In any case, I suppose the idea is that in the long run, most (if not all) of these defines should be moved to platfrom_defaults, right?

My thinking when it comes to the values in the platform defaults module, is that they should be safe rather than optimal. Let's use the Bolt as an example. The most important thing is that if someone flashes a Bolt and tries to fly it, is to make it reasonably safe. Optimal settings must be defined based on the configuration anyway and should (in my opinion) be set using persistent parameters. If values are pretty optimal as well, even better :-)

Agree!

It is possible that we should add functionality to write a set of parameters from file to persistent memory to make it easy to configure a swarm for instance, but that is another story.

Yes, this could be handy...
But creating a (temporary) platform for the specific swarm configuration is also not so difficult...

[krichardsson]

This is defined only in platform_defaults_[platform].h, for all the platforms individually, and this compiles well even without the autoconf.h included...

In pm_stm32f4.c (the only place I found that uses DEFAULT_BAT_LOW_VOLTAGE), this works because pm.h is included before platform_defaults.h. pm.h includes autoconf.h which solves the problem.

[matejkarasek]

Ah, ok :) Thanks...