-
Notifications
You must be signed in to change notification settings - Fork 41
Implement the MPC use case for Monitored Units #108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Implement the MPC use case for Monitored Units #108
Conversation
186302a to
47947ab
Compare
|
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:
|
|
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? |
e8b7796 to
7e07acf
Compare
a82f86c to
193fc5c
Compare
|
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
left a comment
There was a problem hiding this 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!
6834d88 to
62108f5
Compare
|
I've updated the PR with fixes for the simple changes, will look over the rest when I have more time |
DerAndereAndi
left a comment
There was a problem hiding this 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!
62108f5 to
541ba0a
Compare
DerAndereAndi
left a comment
There was a problem hiding this 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 :)
There was a problem hiding this comment.
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)
}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
6363fd7 to
b632337
Compare
…e the suite to MuMpcUsecaseSuite.
- Add more unit tests to increase the coverage
This adds an implementation of the MPC use case for Monitored Units.
The current implementation has 2 major restrictions so far:
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: