Shared static variable in callback_value.h

[moritz-h]

While trying to understand the callback mechanism, I noticed that there is a shared static state between multiple callbacks if they use the callback_value.h template and have the same type. I'm not sure how problematic this is.

Here is the static variable is_buffered_:

bricklib2/utility/callback_value.h

Line 277 in 9e2f400

static bool is_buffered_ = false;

For example the voltage current bricklet is using this function template 3 times for different callbacks, but as it is 3 times the same type they are using the exact same function and therefore the same static variable.
https://github.com/Tinkerforge/voltage-current-v2-bricklet/blob/5014a1eeda48e745d6122213a45b560e59aec1ed/software/src/communication.c#L106-L116

So if i.e. the voltage callback fills the buffer, but cannot send the message, the buffered voltage message may is send during calling the power callback handler. As the message content is also buffered in a static variable at least no wrong message is sent, but the wrong callback handler returns true, which seems to be used for sorting the callback functions here:

bricklib2/utility/communication_callback.c

Lines 39 to 49 in 9e2f400

	for(uint32_t _ = 0; _ < COMMUNICATION_CALLBACK_HANDLER_NUM; _++) {
	if(communication_callbacks[cb_index]()) {
	communication_callback_handler_t current = communication_callbacks[cb_index];
	for(uint32_t i = cb_index; i < COMMUNICATION_CALLBACK_HANDLER_NUM-1; i++) {
	communication_callbacks[i] = communication_callbacks[i+1];
	}
	communication_callbacks[COMMUNICATION_CALLBACK_HANDLER_NUM-1] = current;
	} else {
	cb_index++;
	}
	}

and handling the actual intended callback is delayed by one tick loop.

[borg42]

You are correct, this does not break anything, but it is probably not as originally intended.

The shuffling of the callbacks is indeed for fairness. For example the "temperature image" callback of the Thermal Imaging Bricklet generates enough data to fill the full 64 byte buffer more then 1000x per second (which is the maximum). So if we would not shuffle the callbacks around, it would starve other callbacks that are behind it in the list.

However, i think all callbacks that can generate so much data have complex data types and can't use the CALLBACK_VALUE_SUFFIX stuff. So this bug might not have any real-world effects.

Although i gather that you want to get the maximum amount of data from Voltage/Current Bricklet, so maybe this can have an effect on your use case in some edge cases? If you want to fix this for the Voltage/Current Bricklet, maybe it would make sense to create a CALLBACK_VALUE_SUFFIX_FAIR function that takes bool is_buffered and CALLBACK_VALUE_SUFFIX(CallbackValue_Callback) cb as a parameter. So you would have something like:

bool handle_current_callback(void) {
    static bool is_buffered;
    static CALLBACK_VALUE_SUFFIX(CallbackValue_Callback) cb;
    return handle_callback_value_callback_int32_t_fair(&callback_value_current, FID_CALLBACK_CURRENT, &is_buffered, &cb);
}

The old non-fair function can then call the fair function internally, so we don't have the complete callback handling duplicated.

We could then change to the fair handling step by step every time we change something in the firmware of a Bricklet that has the old handling. With some Bricklets we might need to look at the RAM/flash usage, since we have some Bricklets that use the available resources up completely. So i wouldn't recommend to try to somehow fix this for all Bricklets that use this at once, that could end up taking a lot of time :).

[moritz-h]

Thanks for the explanations. My usecase is not affected by this, only observed it as theoretical problem while reading the code in order to understand what I need to do in Tinkerforge/voltage-current-v2-bricklet#1.

I thought it may be simply possible to move the static values to the state struct here

bricklib2/utility/callback_value.h

Lines 86 to 101 in 08546c2

	typedef struct {
	CALLBACK_VALUE_SUFFIX(CallbackValueGetter) get_callback_value;
	CALLBACK_VALUE_T value_last;
	uint32_t period;
	uint32_t last_time;
	bool value_has_to_change;
	CALLBACK_VALUE_T threshold_min;
	CALLBACK_VALUE_T threshold_max;
	char threshold_option;
	CALLBACK_VALUE_T threshold_min_user;
	CALLBACK_VALUE_T threshold_max_user;
	char threshold_option_user;
	} CALLBACK_VALUE_SUFFIX(CallbackValue);

This is at least what I did now in the mentioned PR for the new functions of the voltage current bricklet, but maybe that is the solution what you meant by increased memory usage.

Anyway, I'm not affected by this, just wanted to share my findings.