Skip to content

Conversation

sthelen-enqs
Copy link
Contributor

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.

@DerAndereAndi
Copy link
Member

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 DerAndereAndi added this to the Version 0.8 milestone Jan 28, 2025
@sthelen-enqs sthelen-enqs force-pushed the wip/failsafe-limit-write-approval branch from cdd7418 to 7be66c0 Compare February 13, 2025 12:14
@sthelen-enqs sthelen-enqs marked this pull request as ready for review April 4, 2025 09:36
@sthelen-enqs
Copy link
Contributor Author

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 sthelen-enqs changed the title WIP: add write approval callbacks for Failsafe values of CS-LPC usecase Add write approval callbacks for Failsafe values of CS-LPC usecase Apr 4, 2025
LucaRoth-EnQS and others added 6 commits May 19, 2025 12:46
… feature.

Made implementation of approval mechansim more general towards the device configuration feature.
This allows to more easily check for what a new value is trying to be set and what kind of value is being set (i.e. a duration, a bool, string etc.)
@sthelen-enqs sthelen-enqs force-pushed the wip/failsafe-limit-write-approval branch from 7be66c0 to dc13778 Compare May 19, 2025 10:47
@DerAndereAndi
Copy link
Member

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

@sthelen-enqs sthelen-enqs force-pushed the wip/failsafe-limit-write-approval branch from c62b1bf to 07809b9 Compare July 14, 2025 15:48
@sthelen-enqs
Copy link
Contributor Author

  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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants