Skip to content

Conversation

@sthelen-enqs
Copy link
Contributor

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

The current implementation has 2 major restrictions so far:

  1. it automatically configures itself as a 3-phase meter (ABC) and there's no way to specify a different configuration right now
  2. 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

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

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
Copy link
Member

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?

@DerAndereAndi DerAndereAndi added the enhancement New feature or request label Oct 15, 2024
@sthelen-enqs 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
Copy link
Contributor Author

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

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

@DerAndereAndi DerAndereAndi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Copy link
Member

@DerAndereAndi DerAndereAndi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great improvements already, thank you!

Copy link
Member

@DerAndereAndi DerAndereAndi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are getting there :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}

@sthelen-enqs sthelen-enqs force-pushed the feat/implement-mu-mpc branch from 6363fd7 to b632337 Compare October 23, 2025 12:12
@sthelen-enqs sthelen-enqs self-assigned this Oct 23, 2025
mohamedeltawel and others added 2 commits November 3, 2025 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants