eebus-go icon indicating copy to clipboard operation
eebus-go copied to clipboard

Add write approval callbacks for Failsafe values of CS-LPC usecase

Open sthelen-enqs opened this issue 1 year ago • 4 comments

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.

sthelen-enqs avatar Jan 07 '25 13:01 sthelen-enqs

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.

DerAndereAndi avatar Jan 22 '25 20:01 DerAndereAndi

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 avatar Apr 04 '25 09:04 sthelen-enqs

@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:

  1. 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)
  1. 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

  1. 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
  }
  1. 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.

DerAndereAndi avatar Jul 14 '25 06:07 DerAndereAndi

  1. The logging issue should be fixed now
  2. I updated the documentation in events.go for LPC/LPP with the new methods as well.
  3. 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.
  4. I'll double-check the test coverage for this

sthelen-enqs avatar Jul 14 '25 15:07 sthelen-enqs