Surface the device write result and fail closed on ambiguous ids

Point 5: the write methods discarded the message counter and reported
success as soon as the request was sent, even if the device later
rejected it. Return the message counter and accept a resultCB so a
non-zero ResultData.ErrorNumber surfaces as a device rejection, matching
the OPEV/LPC/OHPCF write pattern.

Point 6: systemFunctionId and overrunId returned the first matching
entry. Return ErrDataNotAvailable unless exactly one DHW system function
/ one-time-DHW overrun matches, so the wrong entry is never controlled.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: andig

usecases/api/ca_cdsf.go

@@ -3,6 +3,7 @@ package api
 import (
 	"github.com/enbility/eebus-go/api"
 	spineapi "github.com/enbility/spine-go/api"
+	"github.com/enbility/spine-go/model"
 )
 
 // Actor: Configuration Appliance
@@ -37,36 +38,49 @@ type CaCDSFInterface interface {
 	// parameters:
 	//   - entity: the entity of the DHW circuit
 	//   - mode: the DHW operation mode to set
+	//   - resultCB: callback for the device result; a non-zero ResultData.ErrorNumber signals a rejected write
 	//
 	// possible errors:
 	//   - ErrNotSupported if the operation mode is not changeable or not supported
 	//   - ErrDataNotAvailable if the required data is not (yet) available
 	//   - and others
-	WriteOperationMode(entity spineapi.EntityRemoteInterface, mode HvacOperationModeType) error
+	WriteOperationMode(
+		entity spineapi.EntityRemoteInterface,
+		mode HvacOperationModeType,
+		resultCB func(result model.ResultDataType, msgCounter model.MsgCounterType),
+	) (*model.MsgCounterType, error)
 
 	// Scenario 2
 
 	// start the one-time DHW loading overrun of the DHW circuit
 	//
 	// parameters:
 	//   - entity: the entity of the DHW circuit
+	//   - resultCB: callback for the device result; a non-zero ResultData.ErrorNumber signals a rejected write
 	//
 	// possible errors:
 	//   - ErrNotSupported if the overrun status is not changeable
 	//   - ErrDataNotAvailable if the required data is not (yet) available
 	//   - and others
-	StartOneTimeDhw(entity spineapi.EntityRemoteInterface) error
+	StartOneTimeDhw(
+		entity spineapi.EntityRemoteInterface,
+		resultCB func(result model.ResultDataType, msgCounter model.MsgCounterType),
+	) (*model.MsgCounterType, error)
 
 	// Scenario 3
 
 	// stop the one-time DHW loading overrun of the DHW circuit
 	//
 	// parameters:
 	//   - entity: the entity of the DHW circuit
+	//   - resultCB: callback for the device result; a non-zero ResultData.ErrorNumber signals a rejected write
 	//
 	// possible errors:
 	//   - ErrNotSupported if the overrun status is not changeable
 	//   - ErrDataNotAvailable if the required data is not (yet) available
 	//   - and others
-	StopOneTimeDhw(entity spineapi.EntityRemoteInterface) error
+	StopOneTimeDhw(
+		entity spineapi.EntityRemoteInterface,
+		resultCB func(result model.ResultDataType, msgCounter model.MsgCounterType),
+	) (*model.MsgCounterType, error)
 }

usecases/ca/cdsf/public.go

@@ -4,6 +4,7 @@ import (
 	"github.com/enbility/eebus-go/api"
 	"github.com/enbility/eebus-go/features/client"
 	ucapi "github.com/enbility/eebus-go/usecases/api"
+	"github.com/enbility/ship-go/logging"
 	spineapi "github.com/enbility/spine-go/api"
 	"github.com/enbility/spine-go/model"
 	"github.com/enbility/spine-go/util"
@@ -86,31 +87,38 @@ func (e *CDSF) CurrentOperationMode(entity spineapi.EntityRemoteInterface) (ucap
 }
 
 // set the DHW operation mode of the DHW circuit,
-// returns ErrNotSupported if the operation mode is not changeable or not supported
-func (e *CDSF) WriteOperationMode(entity spineapi.EntityRemoteInterface, mode ucapi.HvacOperationModeType) error {
+// returns ErrNotSupported if the operation mode is not changeable or not supported.
+//
+// The returned message counter and the resultCB let the caller observe the
+// device result: a non-zero ResultData.ErrorNumber signals a rejected write.
+func (e *CDSF) WriteOperationMode(
+	entity spineapi.EntityRemoteInterface,
+	mode ucapi.HvacOperationModeType,
+	resultCB func(result model.ResultDataType, msgCounter model.MsgCounterType),
+) (*model.MsgCounterType, error) {
 	if !e.IsCompatibleEntityType(entity) {
-		return api.ErrNoCompatibleEntity
+		return nil, api.ErrNoCompatibleEntity
 	}
 
 	hvac, err := client.NewHvac(e.LocalEntity, entity)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	systemFunctionId, err := e.systemFunctionId(entity)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	data, err := hvac.GetHvacSystemFunctionForId(systemFunctionId)
 	if err != nil {
-		return api.ErrDataNotAvailable
+		return nil, api.ErrDataNotAvailable
 	}
 
 	// only an explicit false blocks the write; an omitted flag is tolerated, as
 	// some devices accept the write without advertising the changeability flag
 	if data.IsOperationModeIdChangeable != nil && !*data.IsOperationModeIdChangeable {
-		return api.ErrNotSupported
+		return nil, api.ErrNotSupported
 	}
 
 	// resolve the requested mode through the DHW system-function relation, so a
@@ -120,7 +128,7 @@ func (e *CDSF) WriteOperationMode(entity spineapi.EntityRemoteInterface, mode uc
 	}
 	relations, err := hvac.GetHvacSystemFunctionOperationModeRelationsForFilter(relationFilter)
 	if err != nil || len(relations) == 0 {
-		return api.ErrDataNotAvailable
+		return nil, api.ErrDataNotAvailable
 	}
 
 	var modeId *model.HvacOperationModeIdType
@@ -140,7 +148,7 @@ func (e *CDSF) WriteOperationMode(entity spineapi.EntityRemoteInterface, mode uc
 		}
 	}
 	if modeId == nil {
-		return api.ErrNotSupported
+		return nil, api.ErrNotSupported
 	}
 
 	writeData := []model.HvacSystemFunctionDataType{
@@ -150,49 +158,63 @@ func (e *CDSF) WriteOperationMode(entity spineapi.EntityRemoteInterface, mode uc
 		},
 	}
 
-	_, err = hvac.WriteHvacSystemFunctionListData(writeData)
+	msgCounter, err := hvac.WriteHvacSystemFunctionListData(writeData)
+	e.registerResultCallback(hvac, msgCounter, resultCB)
 
-	return err
+	return msgCounter, err
 }
 
 // Scenario 2
 
 // start the one-time DHW loading overrun of the DHW circuit,
-// returns ErrNotSupported if the overrun status is not changeable
-func (e *CDSF) StartOneTimeDhw(entity spineapi.EntityRemoteInterface) error {
-	return e.writeOverrunStatus(entity, model.HvacOverrunStatusTypeActive)
+// returns ErrNotSupported if the overrun status is not changeable.
+//
+// The returned message counter and the resultCB let the caller observe the
+// device result: a non-zero ResultData.ErrorNumber signals a rejected write.
+func (e *CDSF) StartOneTimeDhw(
+	entity spineapi.EntityRemoteInterface,
+	resultCB func(result model.ResultDataType, msgCounter model.MsgCounterType),
+) (*model.MsgCounterType, error) {
+	return e.writeOverrunStatus(entity, model.HvacOverrunStatusTypeActive, resultCB)
 }
 
 // Scenario 3
 
 // stop the one-time DHW loading overrun of the DHW circuit,
-// returns ErrNotSupported if the overrun status is not changeable
-func (e *CDSF) StopOneTimeDhw(entity spineapi.EntityRemoteInterface) error {
-	return e.writeOverrunStatus(entity, model.HvacOverrunStatusTypeInactive)
+// returns ErrNotSupported if the overrun status is not changeable.
+//
+// The returned message counter and the resultCB let the caller observe the
+// device result: a non-zero ResultData.ErrorNumber signals a rejected write.
+func (e *CDSF) StopOneTimeDhw(
+	entity spineapi.EntityRemoteInterface,
+	resultCB func(result model.ResultDataType, msgCounter model.MsgCounterType),
+) (*model.MsgCounterType, error) {
+	return e.writeOverrunStatus(entity, model.HvacOverrunStatusTypeInactive, resultCB)
 }
 
 // write the status of the one-time DHW overrun
 func (e *CDSF) writeOverrunStatus(
 	entity spineapi.EntityRemoteInterface,
 	status model.HvacOverrunStatusType,
-) error {
+	resultCB func(result model.ResultDataType, msgCounter model.MsgCounterType),
+) (*model.MsgCounterType, error) {
 	if !e.IsCompatibleEntityType(entity) {
-		return api.ErrNoCompatibleEntity
+		return nil, api.ErrNoCompatibleEntity
 	}
 
 	hvac, err := client.NewHvac(e.LocalEntity, entity)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	overrunId, err := e.overrunId(entity)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	if data, err := hvac.GetHvacOverrunForId(overrunId); err == nil &&
 		data.IsOverrunStatusChangeable != nil && !*data.IsOverrunStatusChangeable {
-		return api.ErrNotSupported
+		return nil, api.ErrNotSupported
 	}
 
 	writeData := []model.HvacOverrunDataType{
@@ -202,12 +224,35 @@ func (e *CDSF) writeOverrunStatus(
 		},
 	}
 
-	_, err = hvac.WriteHvacOverrunListData(writeData)
+	msgCounter, err := hvac.WriteHvacOverrunListData(writeData)
+	e.registerResultCallback(hvac, msgCounter, resultCB)
 
-	return err
+	return msgCounter, err
 }
 
-// return the id of the one-time DHW overrun affecting the DHW system function
+// register a response callback that surfaces the device result of a write to
+// the caller, so a non-zero ResultData.ErrorNumber can be treated as a rejection
+func (e *CDSF) registerResultCallback(
+	hvac *client.Hvac,
+	msgCounter *model.MsgCounterType,
+	resultCB func(result model.ResultDataType, msgCounter model.MsgCounterType),
+) {
+	if resultCB == nil || msgCounter == nil {
+		return
+	}
+
+	cb := func(msg spineapi.ResponseMessage) {
+		if response, ok := msg.Data.(*model.ResultDataType); ok {
+			resultCB(*response, *msgCounter)
+		}
+	}
+	if err := hvac.AddResponseCallback(*msgCounter, cb); err != nil {
+		logging.Log().Debug("failed to add response callback for msgCounter %v: %v", msgCounter, err)
+	}
+}
+
+// return the id of the one-time DHW overrun affecting the DHW system function,
+// returns ErrDataNotAvailable unless exactly one matching overrun exists
 func (e *CDSF) overrunId(entity spineapi.EntityRemoteInterface) (model.HvacOverrunIdType, error) {
 	hvac, err := client.NewHvac(e.LocalEntity, entity)
 	if err != nil {
@@ -227,22 +272,30 @@ func (e *CDSF) overrunId(entity spineapi.EntityRemoteInterface) (model.HvacOverr
 		return 0, api.ErrDataNotAvailable
 	}
 
+	var overrunIds []model.HvacOverrunIdType
 	for _, description := range descriptions {
 		if description.OverrunId == nil {
 			continue
 		}
 
 		for _, affectedId := range description.AffectedSystemFunctionId {
 			if affectedId == systemFunctionId {
-				return *description.OverrunId, nil
+				overrunIds = append(overrunIds, *description.OverrunId)
+				break
 			}
 		}
 	}
 
-	return 0, api.ErrDataNotAvailable
+	// fail closed on an ambiguous result so the wrong overrun is never controlled
+	if len(overrunIds) != 1 {
+		return 0, api.ErrDataNotAvailable
+	}
+
+	return overrunIds[0], nil
 }
 
-// return the id of the DHW system function of the DHW circuit
+// return the id of the DHW system function of the DHW circuit,
+// returns ErrDataNotAvailable unless exactly one matching system function exists
 func (e *CDSF) systemFunctionId(entity spineapi.EntityRemoteInterface) (model.HvacSystemFunctionIdType, error) {
 	hvac, err := client.NewHvac(e.LocalEntity, entity)
 	if err != nil {
@@ -253,7 +306,8 @@ func (e *CDSF) systemFunctionId(entity spineapi.EntityRemoteInterface) (model.Hv
 		SystemFunctionType: util.Ptr(model.HvacSystemFunctionTypeTypeDhw),
 	}
 	descriptions, err := hvac.GetHvacSystemFunctionDescriptionsForFilter(descFilter)
-	if err != nil || len(descriptions) == 0 || descriptions[0].SystemFunctionId == nil {
+	// fail closed on an ambiguous result so the wrong system function is never controlled
+	if err != nil || len(descriptions) != 1 || descriptions[0].SystemFunctionId == nil {
 		return 0, api.ErrDataNotAvailable
 	}
 

usecases/ca/cdsf/public_test.go

@@ -41,34 +41,34 @@ func (s *CaCDSFSuite) Test_CurrentOperationMode() {
 }
 
 func (s *CaCDSFSuite) Test_WriteOperationMode() {
-	err := s.sut.WriteOperationMode(s.mockRemoteEntity, ucapi.HvacOperationModeTypeOn)
+	_, err := s.sut.WriteOperationMode(s.mockRemoteEntity, ucapi.HvacOperationModeTypeOn, nil)
 	assert.NotNil(s.T(), err)
 
-	err = s.sut.WriteOperationMode(s.dhwCircuitEntity, ucapi.HvacOperationModeTypeOn)
+	_, err = s.sut.WriteOperationMode(s.dhwCircuitEntity, ucapi.HvacOperationModeTypeOn, nil)
 	assert.NotNil(s.T(), err)
 
 	s.addHvacData(util.Ptr(true))
 
-	err = s.sut.WriteOperationMode(s.dhwCircuitEntity, ucapi.HvacOperationModeTypeOn)
+	_, err = s.sut.WriteOperationMode(s.dhwCircuitEntity, ucapi.HvacOperationModeTypeOn, nil)
 	assert.Nil(s.T(), err)
 
 	// an unsupported mode cannot be written
-	err = s.sut.WriteOperationMode(s.dhwCircuitEntity, ucapi.HvacOperationModeType("invalid"))
+	_, err = s.sut.WriteOperationMode(s.dhwCircuitEntity, ucapi.HvacOperationModeType("invalid"), nil)
 	assert.NotNil(s.T(), err)
 }
 
 func (s *CaCDSFSuite) Test_WriteOperationMode_NotChangeable() {
 	s.addHvacData(util.Ptr(false))
 
-	err := s.sut.WriteOperationMode(s.dhwCircuitEntity, ucapi.HvacOperationModeTypeOn)
+	_, err := s.sut.WriteOperationMode(s.dhwCircuitEntity, ucapi.HvacOperationModeTypeOn, nil)
 	assert.NotNil(s.T(), err)
 }
 
 func (s *CaCDSFSuite) Test_WriteOperationMode_ChangeabilityOmitted() {
 	// a device may omit the changeability flag but still accept the write
 	s.addHvacData(nil)
 
-	err := s.sut.WriteOperationMode(s.dhwCircuitEntity, ucapi.HvacOperationModeTypeOn)
+	_, err := s.sut.WriteOperationMode(s.dhwCircuitEntity, ucapi.HvacOperationModeTypeOn, nil)
 	assert.Nil(s.T(), err)
 }
 
@@ -101,34 +101,58 @@ func (s *CaCDSFSuite) Test_WriteOperationMode_UnrelatedMode() {
 	assert.Nil(s.T(), fErr)
 
 	// the relation only lists modes 1, 2, 3, so the unrelated mode must not be written
-	err := s.sut.WriteOperationMode(s.dhwCircuitEntity, ucapi.HvacOperationModeTypeOff)
+	_, err := s.sut.WriteOperationMode(s.dhwCircuitEntity, ucapi.HvacOperationModeTypeOff, nil)
 	assert.NotNil(s.T(), err)
 }
 
 func (s *CaCDSFSuite) Test_StartStopOneTimeDhw() {
-	err := s.sut.StartOneTimeDhw(s.mockRemoteEntity)
+	_, err := s.sut.StartOneTimeDhw(s.mockRemoteEntity, nil)
 	assert.NotNil(s.T(), err)
 
-	err = s.sut.StartOneTimeDhw(s.dhwCircuitEntity)
+	_, err = s.sut.StartOneTimeDhw(s.dhwCircuitEntity, nil)
 	assert.NotNil(s.T(), err)
 
 	s.addHvacData(util.Ptr(true))
 
-	err = s.sut.StartOneTimeDhw(s.dhwCircuitEntity)
+	_, err = s.sut.StartOneTimeDhw(s.dhwCircuitEntity, nil)
 	assert.NotNil(s.T(), err)
 
 	s.addOverrunData(true)
 
-	err = s.sut.StartOneTimeDhw(s.dhwCircuitEntity)
+	_, err = s.sut.StartOneTimeDhw(s.dhwCircuitEntity, nil)
 	assert.Nil(s.T(), err)
 
-	err = s.sut.StopOneTimeDhw(s.dhwCircuitEntity)
+	_, err = s.sut.StopOneTimeDhw(s.dhwCircuitEntity, nil)
 	assert.Nil(s.T(), err)
 
 	// the overrun status marked not changeable cannot be written
 	s.addOverrunData(false)
 
-	err = s.sut.StartOneTimeDhw(s.dhwCircuitEntity)
+	_, err = s.sut.StartOneTimeDhw(s.dhwCircuitEntity, nil)
+	assert.NotNil(s.T(), err)
+}
+
+func (s *CaCDSFSuite) Test_WriteOperationMode_AmbiguousSystemFunction() {
+	s.addHvacData(util.Ptr(true))
+
+	// two DHW system functions make the id ambiguous, so the write must fail
+	rFeature := s.remoteDevice.FeatureByEntityTypeAndRole(s.dhwCircuitEntity, model.FeatureTypeTypeHvac, model.RoleTypeServer)
+	descData := &model.HvacSystemFunctionDescriptionListDataType{
+		HvacSystemFunctionDescriptionData: []model.HvacSystemFunctionDescriptionDataType{
+			{
+				SystemFunctionId:   util.Ptr(model.HvacSystemFunctionIdType(1)),
+				SystemFunctionType: util.Ptr(model.HvacSystemFunctionTypeTypeDhw),
+			},
+			{
+				SystemFunctionId:   util.Ptr(model.HvacSystemFunctionIdType(2)),
+				SystemFunctionType: util.Ptr(model.HvacSystemFunctionTypeTypeDhw),
+			},
+		},
+	}
+	_, fErr := rFeature.UpdateData(true, model.FunctionTypeHvacSystemFunctionDescriptionListData, descData, nil, nil)
+	assert.Nil(s.T(), fErr)
+
+	_, err := s.sut.WriteOperationMode(s.dhwCircuitEntity, ucapi.HvacOperationModeTypeOn, nil)
 	assert.NotNil(s.T(), err)
 }