implement subscription

[p-h-a-i-l]

subscribe to recurring updates of cmds.

I don't like, that now every callback has to get the PROTOCOL_STAT pointer.. Do you have a better idea?

[btsimonh]

I like it. especially the simplicity of just turning a subscription into a pseudo read request :).

Set some minimum & default on the subscription period... else we could kill it easily.

Kind of a side discussion: 1/ Actually, when looking through (and from a previous thought): if (params[i].preread) params[i].preread(s); could even be if (params[i].preread) params[i].preread(s, i); or even if (params[i].preread) params[i].preread(s, &params[i]); or even if (params[i].fn) params[i].fn( s, &params[i], FN_TYPE_PREREAD );

This would then mean that some of the functions could be more generic with knowledge of the data they were acting on, rather than having to have separate identical (or similar) functions. The last one would simplify the structure and gather functions together, at a small performance loss.

2/ A concept of time may become important.

e.g. if subscribing to hall position, the tick at the read time should be in the data to give us a feeling for accuracy (or ability to predict position now?

This may then need a subscription to 'time' (heartbeat?). This may need an alternative approach of either:

Only send the time message if the raw tx buffer is empty.

or

Set a flag in the time message if the tx buffer was empty.

But both of these indicate that the time message is 'special' - because the raw send function must recognise it to make the distinction between delayed and undelayed.

3/ the actual data read/write definition does not belong here?

I am conscious that the data we read and write, and operations we perform are application specific, and don't fit so well within the protocol repo :(. As a longer term consideration, maybe we should define HOW data is read/written here (i.e. the PARAMSTAT structure, and operational methods), but provide for 'codes' to be added to the structure from the main repo. - e.g. the protocol repo may add it's own codes for protocol stats read/reset, the flash may (in flashinit) add it's own codes for flash read/write, sensor code could add it's own codes, etc.

If you agree, then we'll add a project task to be addressed at our leisure later... ?

[p-h-a-i-l]

Set some minimum & default on the subscription period... else we could kill it easily.

Was thinking about that too. But what is a good minimum? 1 ? 10 ? (time unit is 1ms here, I'm writing it like this, because I define HAL_TICK manually on non stm32 controllers.)

Kind of a side discussion: 1/ Actually, when looking through (and from a previous thought): if (params[i].preread) params[i].preread(s); could even be if (params[i].preread) params[i].preread(s, i); or even if (params[i].preread) params[i].preread(s, &params[i]); or even if (params[i].fn) params[i].fn( s, &params[i], FN_TYPE_PREREAD );

This would then mean that some of the functions could be more generic with knowledge of the data they were acting on, rather than having to have separate identical (or similar) functions. The last one would simplify the structure and gather functions together, at a small performance loss.

I like the last one, I was almost adding another prereceivedreadfunction, but didn't want to blow up PARAMSTAT anymore. Let's do it.

2/ A concept of time may become important.

e.g. if subscribing to hall position, the tick at the read time should be in the data to give us a feeling for accuracy (or ability to predict position now?

Hmmm. right now "time" is local.

This may then need a subscription to 'time' (heartbeat?). This may need an alternative approach of either:

Only send the time message if the raw tx buffer is empty.

or

Set a flag in the time message if the tx buffer was empty.

But both of these indicate that the time message is 'special' - because the raw send function must recognise it to make the distinction between delayed and undelayed.

Let's put this into a new Issue/Feature proposal.

3/ the actual data read/write definition does not belong here?

I am conscious that the data we read and write, and operations we perform are application specific, and don't fit so well within the protocol repo :(. As a longer term consideration, maybe we should define HOW data is read/written here (i.e. the PARAMSTAT structure, and operational methods), but provide for 'codes' to be added to the structure from the main repo. - e.g. the protocol repo may add it's own codes for protocol stats read/reset, the flash may (in flashinit) add it's own codes for flash read/write, sensor code could add it's own codes, etc.

Yes, I already started to modify the callbacks in my control code during runtime. We could move the callbacks and variable definitions out of this repo into a application specific file. Just a blind #include?

If you agree, then we'll add a project task to be addressed at our leisure later... ?

Ok.

[p-h-a-i-l]

tested, works for me!

[p-h-a-i-l]

FYI, started rewriting to if (params[i].fn) params[i].fn( s, &params[i], FN_TYPE_PREREAD );

(btw, this looks wrong:

void PreRead_getspeeds(PROTOCOL_STAT *s){
    speedsx.speedl = SpeedData.wanted_speed_mm_per_sec[0];
    speedsx.speedr = SpeedData.wanted_speed_mm_per_sec[1];
}

)

[btsimonh]

yes, not even sure speedsx is used. At first I was thinking to give a reduced size in the return, but in the end did not.

[p-h-a-i-l]

done. First step without using a separate file for application specific functions.

Btw. We could skip allocationg memory for the locally used variables now ;)

And we should really push to master soon.. I'm losing track of branches. I pushed to yours now..