Add write approval callbacks for Failsafe values of CS-LPC usecase
This is a work-in-progress proposal for adding write approval callbacks for the failsafe values of the CS-LPC usecase.
Code changes should be functional, but are not yet final. Cleanup and thorough testing may still be required.
Fixes/updates from our side should come when I can find the time to do so. If someone wants to use/update this in the meantime, feel free.
Comments/fixes etc are welcome.
Thanks for starting the work on this. I do now think that the write approval callback API should be used for all writes on any server feature.
I was waiting for some final qualification tests to ensure that a "spec-compliant" implementation can be achieved for LPC/LPP with this PR. The final test is still outstanding due to delays with the qualification, but I'm reasonably confident that no further changes are necessary to this PR to fully pass the qualification.
@sthelen-enqs I analyzed the PR with Claude and iterated a bit over it. Here is its summary:
Based on my analysis, the PR implementation is well-designed and follows the established patterns correctly. Here are my recommendations:
- Fix the Logging Issue
In deviceConfigurationWriteCB, fix the format string:
// Current (line 228)
logging.Log().Debug("LPC deviceConfigurationWriteCB: no device configuration for KeyID %d found")
// Should be:
logging.Log().Debug("LPC deviceConfigurationWriteCB: no device configuration for KeyID found: ", *deviceKeyValueData.KeyId)
- Add Documentation
Add a comment in the interface or README explaining the approval pattern:
// WriteApprovalRequired event is fired when any server feature receives a write request // that requires approval. The application should: // 1. Check PendingConsumptionLimits() for LoadControl writes // 2. Check PendingDeviceConfigurations() for DeviceConfiguration writes // 3. Call the appropriate ApproveOrDeny* method with the msgCounter
- Consider Adding Helper Methods (Optional)
For better developer experience, consider adding helper methods to check if specific configurations are pending:
// Check if a specific configuration key is pending approval
func (e *LPC) IsPendingDeviceConfiguration(msgCounter model.MsgCounterType, keyName model.DeviceConfigurationKeyNameType) bool {
configs := e.PendingDeviceConfigurations()
if pendingConfigs, exists := configs[msgCounter]; exists {
for _, config := range pendingConfigs {
if config.Description != nil && config.Description.KeyName != nil &&
*config.Description.KeyName == keyName {
return true
}
}
}
return false
}
- Test Coverage
Ensure tests cover:
- Approval flow for both parameters
- Auto-approval of unrelated DeviceConfiguration writes
- Handling multiple configuration changes in one message
- Thread safety of concurrent approvals
- Timeout behavior (inherited from spine-go)
Summary
The PR implementation is functionally complete and well-designed. It correctly:
- Follows the established LoadControl pattern
- Provides selective filtering for relevant parameters
- Maintains thread safety
- Gives applications full control over approval decisions
- Auto-approves unrelated writes to avoid blocking other use cases
The only required change is the logging fix. The other suggestions are optional improvements for developer experience and documentation.
- The logging issue should be fixed now
- I updated the documentation in events.go for LPC/LPP with the new methods as well.
- I don't think point 3 is particularly relevant since you need to check every pending device configuration (and then switch on the type) in the handler, I wouldn't recommend selectively handling device configurations.
- I'll double-check the test coverage for this