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

[settings] Add getters #3307

Open
Fabien-B opened this issue Jun 14, 2024 · 4 comments
Open

[settings] Add getters #3307

Fabien-B opened this issue Jun 14, 2024 · 4 comments

Comments

@Fabien-B
Copy link
Contributor

In the settings, we have the handler that allows to call a function to set the value.
We need the same mecanism to get the value: a getter.

This getter should return a float, to be consistent with the current system.

Current situation

Let's take an example with this setting:

<dl_setting var="autopilot.kill_throttle" min="0" step="1" max="1" module="autopilot" values="Resurrect|Kill" handler="KillThrottle"/>
#define DlSetting(_idx, _value) { \
  switch (_idx) { \
     ...
    case 13: autopilot_KillThrottle( _value ); _value = autopilot.kill_throttle; break;\
    }\
}

#define PeriodicSendDlValue(_trans, _dev) { \
     ...
    case 13: var = autopilot.kill_throttle; break;\
    ...
}

static inline float settings_get_value(uint8_t i) {
  ...
  case 13: return autopilot.kill_throttle;
  ...
}

⚠️ First of all even in the current system, I think _value = autopilot.kill_throttle; (line 4) should be removed: If there is a handler, the value should not be forcefully changed after calling the handler.

Proposed new behavior

The setting could be changed like this:

<dl_setting name="KillThrottle" getter="getKillThrottle()" min="0" step="1" max="1" values="Resurrect|Kill" handler="KillThrottle"/>

It would result in this generated code:

#define DlSetting(_idx, _value) { \
  switch (_idx) { \
     ...
    case 13: KillThrottle( _value ); break;\
    }\
}

#define PeriodicSendDlValue(_trans, _dev) { \
     ...
    case 13: var = getKillThrottle(); break;\
    ...
}

static inline float settings_get_value(uint8_t i) {
  ...
  case 13: return getKillThrottle();
  ...
}

Defining the getter like this getter="getKillThrottle()" allows to specify either a function call, or a global variable like in the current situation.

The name attribute would be added to define the name of the setting. shortname would be deprecated in favor of name.
The name resolution order would be: name, shortname (with deprecated warning), var.

XML valid cases

<dl_setting var="autopilot.kill_throttle" min="0" step="1" max="1" values="Resurrect|Kill"/>
<dl_setting name="KillThrottle" var="autopilot.kill_throttle" min="0" step="1" max="1" values="Resurrect|Kill" handler="KillThrottle"/>
<dl_setting name="KillThrottle" getter="getKillThrottle()" min="0" step="1" max="1" values="Resurrect|Kill" handler="KillThrottle"/>
<dl_setting name="KillThrottle" var="autopilot.kill_throttle" getter="getKillThrottle()" min="0" step="1" max="1" values="Resurrect|Kill"/>

The following should Fail at compilation, because var would be unused:

<dl_setting var="autopilot.kill_throttle" getter="getKillThrottle()" min="0" step="1" max="1" values="Resurrect|Kill" handler="KillThrottle"/>
@fvantienen
Copy link
Member

I like this idea :)

@dewagter
Copy link
Member

Hi @Fabien-B, this looks nice and very coherent. I'm just missing which situation is not well handled with the current setup?

@Fabien-B
Copy link
Contributor Author

Fabien-B commented Jun 18, 2024

Hi, there are multiple times where a variable is not needed, but you need to define it anyway because of how settings works currently.
Here is some awful code I wrote recently:

// settings
extern int tag_tracking_setting_id;
extern float tag_tracking_motion_type;
extern float tag_tracking_predict_time;
extern float tag_tracking_kp;
extern float tag_tracking_kpz;
void tag_tracking_set_setting_id(float id);
void tag_tracking_set_motion_type(float motion_type);
void tag_tracking_set_predict_time(float predict_time);
void tag_tracking_set_kp(float kp);
void tag_tracking_set_kpz(float kpz);

Here the variable is useless:
void usbStorage_enable_usb_storage(float e) {
if(e > 0.5 && palReadPad(SDLOG_USB_VBUS_PORT, SDLOG_USB_VBUS_PIN) == PAL_HIGH) {
chBSemSignal(&bs_start_msd);
} else {
usb_storage_status = 0;
}
}

And in a more general way, the current system forces us to have global variables everywhere, which is a bad design choice.
Sometimes we access directly these variables instead of using the dedicated functions (or macro) that may do essential checks.
Also, it's very difficult to understand how paparazzi works because of these global variables.

We can't change everything in paparazzi at once, but a change like this can allow us to slowly shift towards cleaner code.

@dewagter
Copy link
Member

Thanks a lot. Great suggestion. I'll investigate which unnecessary variables we introduced once this is merged.

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

No branches or pull requests

3 participants