Conversation
klew
left a comment
klew
left a comment
There was a problem hiding this comment.
Beside of below comments, we should define how to handle "read-write" variant of GPT, i.e. the same channel type, but different function? Do we allow user to change the function? If yes, how to define which functions are supported by the device? etc.
Please discuss open questions before making further changes.
| char text[SUPLA_GENERAL_PURPOSE_TEXT_MAX_SIZE + 1] = {}; | ||
| bool valueChanged = false; | ||
| uint32_t keepHistory = 0; | ||
| uint16_t refreshIntervalMs = 0; |
|
|
||
| private: | ||
| ChannelExtended channel; | ||
| char text[SUPLA_GENERAL_PURPOSE_TEXT_MAX_SIZE + 1] = {}; |
There was a problem hiding this comment.
there is no need to keep text copy here. You already have a channel member, which can hold text value
| #define SUPLA_GENERAL_PURPOSE_MEASUREMENT_CHART_TYPE_CANDLE 2 | ||
|
|
||
| #define SUPLA_GENERAL_PURPOSE_UNIT_SIZE 15 | ||
| #define SUPLA_GENERAL_PURPOSE_TEXT_MAX_SIZE 255 // ver. >= 29 |
There was a problem hiding this comment.
add a static_assert to proto_check.cpp that ensure that this define is <= SUPLA_CHANNELEXTENDEDVALUE_SIZE
| // Keep history: 0 = no (default), 1 = yes | ||
| unsigned char KeepHistory; | ||
| unsigned char Reserved[13]; | ||
| } TChannelConfig_GeneralPurposeText; // v. >= 29 |
There was a problem hiding this comment.
this is not used currenlty. If no device shared configuration is needed, then maybe we don't need to define it and share it with a device?
What other configuration options may be added later?
| if (text == nullptr) { | ||
| text = ""; | ||
| } | ||
| size_t len = strnlen(text, SUPLA_GENERAL_PURPOSE_TEXT_MAX_SIZE); | ||
| bool same = (strncmp(this->text, text, SUPLA_GENERAL_PURPOSE_TEXT_MAX_SIZE) == 0); | ||
| if (!same) { | ||
| memset(this->text, 0, sizeof(this->text)); | ||
| memcpy(this->text, text, len); | ||
| valueChanged = true; | ||
| } |
There was a problem hiding this comment.
this logic should be added to ChannelExtended class (see setNewValue method in channel.h/cpp for standard channels - it performs check if value was changed and handles "value changed" flag on Channel class level).
| // Increment a persistent 64-bit sequence counter stored in the 8-byte | ||
| // channel value so the server always sees a changed value and sends it. | ||
| ++seqCounter; | ||
| channel.setNewValue(reinterpret_cast<const char *>(&seqCounter)); |
There was a problem hiding this comment.
this can be also handled internally in ChannelExtended class withint some setNewValue override (or new method like setNewTextValue in order to avoid overload of current const char * variant)
| ApplyConfigResult GeneralPurposeText::applyChannelConfig( | ||
| TSD_ChannelConfig *newConfig, bool local) { | ||
| (void)local; | ||
| if (!newConfig) { | ||
| return ApplyConfigResult::NotSupported; | ||
| } | ||
| if (newConfig->ConfigType != SUPLA_CONFIG_TYPE_DEFAULT) { | ||
| return ApplyConfigResult::NotSupported; | ||
| } | ||
| if (newConfig->ConfigSize < sizeof(TChannelConfig_GeneralPurposeText)) { | ||
| SUPLA_LOG_WARNING("GPT[%d]: config too short", getChannelNumber()); | ||
| return ApplyConfigResult::NotSupported; | ||
| } | ||
| auto cfg = reinterpret_cast<TChannelConfig_GeneralPurposeText *>( | ||
| newConfig->Config); | ||
| keepHistory = cfg->KeepHistory; | ||
| refreshIntervalMs = cfg->RefreshIntervalMs; | ||
| SUPLA_LOG_INFO( | ||
| "GPT[%d]: config applied: keepHistory=%u refreshIntervalMs=%u", | ||
| getChannelNumber(), keepHistory, refreshIntervalMs); | ||
| return ApplyConfigResult::Success; | ||
| } |
|
Please also add some tests that covers new GPT channel. Consider adding support to sd4linux as well. |
This branch has not been deployed
2 participants
What does this PR change?
Introducing new channel that allows to pass text - General Purpose Text channel
Affected area
Checklist