Refactor display settings handling in BLE callback

[NeilHanlon]

I wrote the following in #1592 , but it's not great.

IronOS/source/Core/BSP/Pinecilv2/ble_handlers.cpp

Lines 246 to 255 in 68d18a4

	// @TODO refactor to make this more usable
	if (uuid_value == SettingsOptions::OLEDInversion) {
	OLED::setInverseDisplay(getSettingValue(SettingsOptions::OLEDInversion));
	}
	if (uuid_value == SettingsOptions::OLEDBrightness){
	OLED::setBrightness(getSettingValue(SettingsOptions::OLEDBrightness));
	}
	if (uuid_value == SettingsOptions::OrientationMode){
	OLED::setRotation(getSettingValue(SettingsOptions::OrientationMode) & 1);
	}

There is probably a better way to handle this than three if statements in a row :)

Open to suggestions

[GPSBabelDeveloper]

Here's three different ways to make that a little less grubby. I prefer the style of the first one, all other things being equal.

switch (uuid_value) {
case SettingsOptions::OLEDInversion:
OLED::setInverseDisplay(getSettingValue(SettingsOptions::OLEDInversion));
break;
case SettingsOptions::OLEDBrightness:
OLED::setBrightness(getSettingValue(SettingsOptions::OLEDBrightness));
break;
case SettingsOptions::OrientationMode:
OLED::setRotation(getSettingValue(SettingsOptions::OrientationMode) & 1);
break;
default:
BT_WARN("Unhandled uuid_value in %s. Got 0x%x", PRETTY_FUNCTION, uuid_value);
break;
}

Or keep it like this, but don't force common subexpression elimination to determine the mutual exclusivity of the paths:

if (uuid_value == SettingsOptions::OLEDInversion) {
OLED::setInverseDisplay(getSettingValue(SettingsOptions::OLEDInversion));
} else if (uuid_value == SettingsOptions::OLEDBrightness) {
OLED::setBrightness(getSettingValue(SettingsOptions::OLEDBrightness));
} else if (uuid_value == SettingsOptions::OrientationMode) {
OLED::setRotation(getSettingValue(SettingsOptions::OrientationMode) & 1);
}

You could possibly shave a few clock cycles off making PMFs to the three methods, masking the bottom two bits from uuid_value and jumping through a function dispatch table with the constant arguments andtesting explictly for the last one and doing something similar after a mask. A self-respecting optimizer MAY do this for you if it can determine these are pure functions (i.e. they're marked const and can be proven to not modify larger scope. If you need clock cycles this badly, you're probably living wrong, but I"m all about giving choices.

Consider marking the functions const:

static void setBrightness(uint8_t contrast) const;
static void setInverseDisplay(bool inverted) const ;
Same for others

... if you can. If you're not modifying any state of the OLED class (e.g. no globals, no honking on the invisible this*) that even further clarifies the code and exposes additional optimizationopportunities.

[discip]

@NeilHanlon
So... are you planning to implement any of the proposed solutions? Or do you expect this to be done for you?
I could try to, if you want.

[River-Mochi]

@NeilHanlon of the new options for the code that GPSBabel suggested to make it better, which one are you going to Submit? it should be submitted asap though so that it can go into the 2.21 release yes?

[NeilHanlon]

Yep, any of these would be fine for implementing. Was planning to get back to this soon, but thank you for taking it @discip !

[discip]

No, this is not implemented yet.
This will automatically close once merged.
It doesn't have to be done manually.

[River-Mochi]

thanks @discip