HasParameterIF and ParameterHelper confusing to use #153

New Issue

muellerr · 2020-08-02T13:52:13+02:00

muellerr commented

2020-08-02 13:52:13 +02:00

I think HasParametersIF is confusing to implement in its current form and even more confusing to understand.

Major issues are confusing naming (Parameter ID contains another Parameter ID, which is accessed via getMatrixId?) , implicit expected structure of parameter packets
(there is a paramerer header which contains type information, column and row, why not use third parameter field inside parameter message to make that expected information explicit to the user?), and no documentation anywhere, at least not in the code.

Also, I still have issues with the concept of getting two wrappers. What wrapper do I use? what is the purpose of newValues?

Also, in getParameter, a uin16_t is passed as the index type, but the getter function returns a uint8_t

Suggestions:

Rename getMatrixId() to getUniqueIdentifierId()
ParameterMessage, requires CommandMessage refactoring (which unlocks more parameter slots): Expose type, column and row explicitely via setter function for parameter load message.
The uint8_t index is actually a linear index running left to right, top to bottom if I deduced that from the code correctly. Also, it is always implicitely converted from uint8_t to uint16_t in ParameterWrapper and ParameterHelper. I suggest to convert it to a uint16_t in HasParametersIF and also document that this is actually a linear index, running from left to right, top to bottom for matrices. This changes the meaning of ParameterId_t: The first byte will be the domain ID, the second byte a unique identifier (I assume that there will not be more than 255 parameters for a software object,that would be bad design..). And the third and the fourth byte are going to be the linear index.
Rename getParameter to loadParameter . A getter always implies read-only, no setting. So this name is already confusing. This will propably be a separatep ull request.

I think `HasParametersIF` is confusing to implement in its current form and even more confusing to understand. Major issues are confusing naming (Parameter ID contains another Parameter ID, which is accessed via getMatrixId?) , implicit expected structure of parameter packets (there is a paramerer header which contains type information, column and row, why not use third parameter field inside parameter message to make that expected information explicit to the user?), and no documentation anywhere, at least not in the code. Also, I still have issues with the concept of getting two wrappers. What wrapper do I use? what is the purpose of `newValues`? Also, in `getParameter`, a `uin16_t` is passed as the index type, but the getter function returns a `uint8_t` Suggestions: 1. Rename `getMatrixId()` to `getUniqueIdentifierId()` 2. `ParameterMessage`, requires CommandMessage refactoring (which unlocks more parameter slots): Expose type, column and row explicitely via setter function for parameter load message. 3. The uint8_t index is actually a linear index running left to right, top to bottom if I deduced that from the code correctly. Also, it is always implicitely converted from uint8_t to uint16_t in `ParameterWrapper` and `ParameterHelper`. I suggest to convert it to a uint16_t in HasParametersIF and also document that this is actually a linear index, running from left to right, top to bottom for matrices. This changes the meaning of ParameterId_t: The first byte will be the domain ID, the second byte a unique identifier (I assume that there will not be more than 255 parameters for a software object,that would be bad design..). And the third and the fourth byte are going to be the linear index. 4. Rename `getParameter` to `loadParameter` . A getter always implies read-only, no setting. So this name is already confusing. This will propably be a separatep ull request.

muellerr added the

feature

Documentation

labels 2020-08-02 13:52:13 +02:00

muellerr changed title from ~~`HasParameterIF` and `ParameterHelper` confusing to use~~ to HasParameterIF and ParameterHelper confusing to use

2020-08-02 13:52:28 +02:00

muellerr added the

API Change

label 2020-10-18 16:58:37 +02:00

muellerr referenced a pull request that will close this issue

2020-12-01 15:31:43 +01:00

Parameter Improvements #250

gaisser referenced this issue

2020-12-03 18:40:51 +01:00

Parameter Improvements #250

gaisser added this to the ASTP 1.0.0 Local pools milestone 2020-12-03 18:41:05 +01:00

gaisser closed this issue