Implement the MPC use case for Monitored Units #108

sthelen-enqs · 2024-08-29T12:43:36Z

This adds an implementation of the MPC use case for Monitored Units.

The current implementation has 2 major restrictions so far:

it automatically configures itself as a 3-phase meter (ABC) and there's no way to specify a different configuration right now
it only supports the required phase-to-neutral and not the phase-to-phase voltages

I'm opening this as a Draft PR because while this UC works, it can still be improved and I'm looking for feedback and potential improvements. Some of the areas I'm explicitly looking for feedback in are:

how best to make instantiation of the use case configurable (number of phases supported etc)
how best to adapt the public usecase API when phases are configurable (e.g. adapting functions to take []float64 or something else entirely)
how to improve the current tests, I thought about connecting a remoteDevice and reading the set values out via that but couldn't get that to work easily, the alternative as far as I can see would be to add functions to the public api and copy the cs/lpc tests

DerAndereAndi · 2024-08-29T17:27:06Z

Thanks a lot for the PR!

Some first thoughts, more will surely come:

Having individual public functions for each scenario is what surely is expected and needed. But using those individually will result in a notify update message for each call, which I think is not nice.

I do see 2 options here:

Add another public method that can take all measurement values for all scenarios, with all being optional at the end so only the ones with actual data will be used
Introducing a mechanism in the SPINE repository to combine multiple local data updates to work like a transaction, where only the transaction end will trigger the actual local data storage update which will then automatically trigger the notify messages to all subscribers

DerAndereAndi · 2024-08-29T17:52:24Z

Regarding initialization: how about the possibility to define the supported scenarios, and for each scenario define a configuration item where items like amount of phases, measurement type, reference to etc. needs to be set.

Scenario 1 is required, all other scenarios are option/recommended. And even within the scenarios there are optional items, e.g. phase specfiic power.

Also I am wondering you only added setters, but maybe it is also helpful to have getters to e.g. validate the current storage?

usecases/api/mu_mpc.go

usecases/mu/mpc/public.go

sthelen-enqs · 2024-10-25T11:26:10Z

I've updated the PR with support for updating multiple measurements at the same time using the Update() function. For an example as to how to use the Update() function look at the Test_PowerPerPhase test.

The mu/mpc use case is now also configurable for e.g. non-three-phase use cases by passing configuration parameters to the NewMpc function. The config types are located in usecases/mu/mpc/config.go. For an example as to how to use these configuration parameters, look at the MPC testhelper

DerAndereAndi

Awesome updates, I really like the way you solve the multiple datapoint update topic!

usecases/usecase/usecase.go

usecases/mu/mpc/config.go

usecases/mu/mpc/usecase.go

DerAndereAndi · 2024-11-01T15:29:02Z

usecases/mu/mpc/usecase.go

+				model.FeatureTypeTypeMeasurement,
+			},
+		},
+	}


The scenarios should only be added, if the configuration for this scenario is provided and hence the application supports it. Right now this code would always report that all scenarios are supported.

Should be fixed now, though it might be nice to have tests for it as well.

Yes. Test coverage still has potential to grow ;)

DerAndereAndi · 2024-11-01T15:48:35Z

usecases/mu/mpc/usecase.go

+		panic(err)
+	}
+
+	idEc1 := model.ElectricalConnectionIdType(0)


This will cause problems, as other use cases may use an eletrical connection in the entity, and they may not be identical.

Instead we should check if one exists with the properties defined in the spec and use that. Otherwise create one.

We added a GetOrAddIdForDescription function to the electricalConnection which checks if there's an ID we can reues and otherwise creates a new id

DerAndereAndi · 2024-11-01T16:04:20Z

usecases/mu/mpc/testhelper_test.go

+	s.sut.AddFeatures()
+	s.sut.AddUseCase()
+
+	//s.remoteDevice, s.monitoredEntity = setupDevices(s.service, s.T())


I would love to see such tests as in CS/LPC to be added, not just using the mocks.

We updated the tests to use Filters to query the stored data representation for the usecases. This should test that querying the data works from the client perspective without needing to reimplement the ma/mpc use cases in the tests for mu/mpc.

sthelen-enqs · 2024-11-04T10:19:43Z

I've updated the PR with fixes for the simple changes, will look over the rest when I have more time

DerAndereAndi

Great improvements already, thank you!

usecases/mu/mpc/config.go

DerAndereAndi · 2024-11-04T10:34:55Z

usecases/mu/mpc/usecase.go

+	return uc, nil
+}
+
+func (e *MPC) AddFeatures() {


This method is getting quite loooooooong. How about splitting it up into individual functions for each config?
Also I am still not sure if using panics is the right approach. Maybe AddFeatures() signature should be changed to AddFeatures() error overall ?

Also the code looks quite repetitive, and maybe there is a good way to shrink this down, and thus making it also easire to improve the test coverage for all error scenarios?

We split the constituent measurement types into separate methods, and adjusted AddFeatures and AddUsecase to return error for all usecases

Co-authored-by: Simon Thelen <[email protected]>

Implements the new approach added by enbility#115 to allow setting measurement values independently or at the same time

This commit also adds some fixes to allow leaving unsupported config parameters to NewMPC as nil and a test for those scenarios.

…lConnectionId for a specific ElectricalConnectionDescription

DerAndereAndi

We are getting there :)

DerAndereAndi · 2024-12-02T14:06:55Z

usecases/mu/mpc/usecase_test.go

This file contains some unused variables in BasicSuite. Would be great to remove or comment them.

DerAndereAndi · 2024-12-02T14:11:53Z

features/server/electricalconnection.go

@@ -31,6 +31,48 @@ func NewElectricalConnection(localEntity spineapi.EntityLocalInterface) (*Electr
 	return ec, nil
 }

+func (e *ElectricalConnection) GetOrAddIdForDescription(


Please add comments for this function to understand its usage

DerAndereAndi · 2024-12-02T14:13:21Z

features/server/electricalconnection.go

+				description.Label == electricalConnectionDescription.Label &&
+				description.Description == electricalConnectionDescription.Description {
+				electricalConnectionId = description.ElectricalConnectionId
+				break


How about returning here directly? That would remove the need for the if statement in line 62

DerAndereAndi · 2024-12-02T14:16:35Z

usecases/cs/lpc/usecase.go

 	// client features
 	_ = e.LocalEntity.GetOrAddFeature(model.FeatureTypeTypeDeviceDiagnosis, model.RoleTypeClient)

 	// server features
 	f := e.LocalEntity.GetOrAddFeature(model.FeatureTypeTypeLoadControl, model.RoleTypeServer)
 	f.AddFunctionType(model.FunctionTypeLoadControlLimitDescriptionListData, true, false)
 	f.AddFunctionType(model.FunctionTypeLoadControlLimitListData, true, true)
-	_ = f.AddWriteApprovalCallback(e.loadControlWriteCB)
+	err := f.AddWriteApprovalCallback(e.loadControlWriteCB)


How about (for consistency) use if err := ...; err != nil ... ?

DerAndereAndi · 2024-12-02T14:18:53Z

usecases/cs/lpc/usecase.go

@@ -204,6 +208,8 @@ func (e *LPC) AddFeatures() {
 			},
 		}
 		_ = lc.UpdateLimitDataForIds(newLimiData)
+	} else {


With these long if statements, it might be easier to read the code if the error and return are handled directly in the if statement.

So changing instances like this into

dcs, err := ... if err != nil { return err }

That also removes the need for an if statement for success.

In addition I think we should check very early for feature existence and exit before adding any data if any of the required features are missing. This applies to all modified usecase.go implementations.

DerAndereAndi · 2024-12-02T14:41:12Z

usecases/cs/lpc/usecase.go

If would love to see added testcases for checking the error handling, e.g. by adding tests to usecase_test.go like:

func (s *CsLPCSuite) Test_AddFeatures() { localEntity := spinemocks.NewEntityLocalInterface(s.T()) s.sut.LocalEntity = localEntity mockLc := spinemocks.NewFeatureLocalInterface(s.T()) mockLc.EXPECT().AddFunctionType(mock.Anything, mock.Anything, mock.Anything).Return() localEntity.EXPECT().GetOrAddFeature(model.FeatureTypeTypeDeviceDiagnosis, model.RoleTypeClient).Return(nil).Once() localEntity.EXPECT().GetOrAddFeature(model.FeatureTypeTypeLoadControl, model.RoleTypeServer).Return(mockLc).Once() expErr := errors.New("error") mockLc.EXPECT().AddWriteApprovalCallback(mock.Anything).Return(expErr).Once() err := s.sut.AddFeatures() assert.NotNil(s.T(), err) }

DerAndereAndi · 2024-12-02T14:44:19Z

usecases/usecase/usecase.go

@@ -114,6 +117,10 @@ func (u *UseCaseBase) IsCompatibleEntityType(entity spineapi.EntityRemoteInterfa
 		return false
 	}

+	if u.allEntityTypesValid {


Please add a test for this scenario, e..g by adding the following in usecase_test.go link 26:

s.uc.allEntityTypesValid = true result = s.uc.IsCompatibleEntityType(payload.Entity) assert.True(s.T(), result)

DerAndereAndi · 2024-12-02T14:45:46Z

usecases/mu/mpc/usecase.go

+	return nil
+}
+
+func (e *MPC) getMeasurementDataForId(id *model.MeasurementIdType) (float64, error) {


Please add tests for this function implementation

DerAndereAndi · 2024-12-02T14:47:18Z

usecases/mu/mpc/usecase.go

+
+	parameterDescriptionId := electricalConnection.AddParameterDescription(parameterDescription)
+	if parameterDescriptionId == nil {
+		return errors.New("could not add parameter description")


These error cases could be tested by passing mocks of server.ElectricalConnection and return an error on the function call.

Applies to all the other private functions as well.

DerAndereAndi · 2024-12-02T14:50:47Z

usecases/mu/mpc/public.go

+	for _, measurementDataForId := range updateData {
+		if !measurementDataForId.Supported() {
+			return measurementDataForId.NotSupportedError()
+		} else {


The else statement is not needed here, as we return in the other case.

So this code could be simplified to:

if !measurementDataForId.Supported() { return measurementDataForId.NotSupportedError() } measurementDataForIds = append(measurementDataForIds, measurementDataForId.MeasurementData())

sthelen-enqs force-pushed the feat/implement-mu-mpc branch from 186302a to 47947ab Compare August 29, 2024 12:47

DAMEK86 reviewed Aug 31, 2024

View reviewed changes

usecases/api/mu_mpc.go Show resolved Hide resolved

sthelen-enqs force-pushed the feat/implement-mu-mpc branch from e8b7796 to 7e07acf Compare September 27, 2024 09:34

DerAndereAndi reviewed Sep 27, 2024

View reviewed changes

usecases/mu/mpc/public.go Show resolved Hide resolved

DerAndereAndi added the enhancement New feature or request label Oct 15, 2024

sthelen-enqs force-pushed the feat/implement-mu-mpc branch 2 times, most recently from a82f86c to 193fc5c Compare October 25, 2024 10:10

sthelen-enqs marked this pull request as ready for review October 25, 2024 11:28

DerAndereAndi requested changes Nov 1, 2024

View reviewed changes

sthelen-enqs force-pushed the feat/implement-mu-mpc branch from 6834d88 to 62108f5 Compare November 4, 2024 10:10

DerAndereAndi requested changes Nov 4, 2024

View reviewed changes

nils-prommersberger and others added 15 commits November 29, 2024 10:55

Implement MPC usecase for the MonitoredUnit actor type

d98505c

Co-authored-by: Simon Thelen <[email protected]>

Add and use allEntityTypesValid boolean instead of nil validEntityTypes

2828ede

Update MPC to allow for updating multiple data points in one step

024d8be

Implements the new approach added by enbility#115 to allow setting measurement values independently or at the same time

Make the mpc configurable. And support option stuff.

1192090

Fix comments and formatting mentioned in review

1cd1b6b

Only add scenarios when they're supported

5f52d28

This commit also adds some fixes to allow leaving unsupported config parameters to NewMPC as nil and a test for those scenarios.

refactor the addFeatures method of the mpc

014519d

Add test for the mpc-power-config PhaseConnection-function

373a3f7

Ignore case for the SupportPhases function of the power config.

86e2b8c

Add more tests with mocks

22f7190

Remove panic statements with an additional wrapper struct.

cd4d9df

little fixes

14c29df

Test if the client-filters work with the data set by the server.

b3e1c56

Let the AddFeatures method return an error

f789ffe

Add a function in the electrical connection to get or add a Electrica…

12654c1

…lConnectionId for a specific ElectricalConnectionDescription

sthelen-enqs added 2 commits November 29, 2024 12:06

Update mocks and fix service_test

f6c0bd8

Check AddFeatures return values in tests

541ba0a

sthelen-enqs force-pushed the feat/implement-mu-mpc branch from 62108f5 to 541ba0a Compare November 29, 2024 11:06

sthelen-enqs added 2 commits November 29, 2024 12:47

Increase test coverage

05a6b42

Address comments during code review

6363fd7

DerAndereAndi requested changes Dec 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the MPC use case for Monitored Units #108

Implement the MPC use case for Monitored Units #108

sthelen-enqs commented Aug 29, 2024

DerAndereAndi commented Aug 29, 2024

DerAndereAndi commented Aug 29, 2024

sthelen-enqs commented Oct 25, 2024

DerAndereAndi left a comment

DerAndereAndi Nov 1, 2024

sthelen-enqs Nov 4, 2024

DerAndereAndi Nov 4, 2024

DerAndereAndi Nov 1, 2024

sthelen-enqs Nov 29, 2024

DerAndereAndi Nov 1, 2024

sthelen-enqs Nov 29, 2024

sthelen-enqs commented Nov 4, 2024

DerAndereAndi left a comment

DerAndereAndi Nov 4, 2024

sthelen-enqs Nov 29, 2024

DerAndereAndi left a comment

DerAndereAndi Dec 2, 2024

DerAndereAndi Dec 2, 2024

DerAndereAndi Dec 2, 2024

DerAndereAndi Dec 2, 2024

DerAndereAndi Dec 2, 2024

DerAndereAndi Dec 2, 2024

DerAndereAndi Dec 2, 2024

DerAndereAndi Dec 2, 2024

DerAndereAndi Dec 2, 2024

DerAndereAndi Dec 2, 2024

Implement the MPC use case for Monitored Units #108

Are you sure you want to change the base?

Implement the MPC use case for Monitored Units #108

Conversation

sthelen-enqs commented Aug 29, 2024

DerAndereAndi commented Aug 29, 2024

DerAndereAndi commented Aug 29, 2024

sthelen-enqs commented Oct 25, 2024

DerAndereAndi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sthelen-enqs commented Nov 4, 2024

DerAndereAndi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DerAndereAndi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment