Skip to content

Comments

Remove support for include_broken_batteries in control commands#713

Merged
shsms merged 3 commits intofrequenz-floss:v1.x.xfrom
shsms:include-broken-batteries
Oct 13, 2023
Merged

Remove support for include_broken_batteries in control commands#713
shsms merged 3 commits intofrequenz-floss:v1.x.xfrom
shsms:include-broken-batteries

Conversation

@shsms
Copy link
Contributor

@shsms shsms commented Oct 9, 2023

This removes

  • the include_broken_batteries fields in request objects and from
    function parameter lists,
  • the caching of battery data, that was necessary when trying to use
    broken batteries,
  • corresponding tests

Closes #665

@shsms shsms requested a review from a team as a code owner October 9, 2023 12:00
@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:data-pipeline Affects the data pipeline part:actor Affects an actor ot the actors utilities (decorator, etc.) part:core Affects the SDK core components (data structures, etc.) part:microgrid Affects the interactions with the microgrid labels Oct 9, 2023
@shsms shsms marked this pull request as draft October 9, 2023 12:01
@shsms
Copy link
Contributor Author

shsms commented Oct 9, 2023

Based on #692, only the last two commits are relevant. Will rebase after #692 is merged.

@shsms shsms added this to the v1.0.0-rc2 milestone Oct 9, 2023
@shsms shsms force-pushed the include-broken-batteries branch from f706dcd to 1b7c7ba Compare October 9, 2023 15:03
@llucax llucax removed this from the v1.0.0-rc2 milestone Oct 10, 2023
@llucax llucax changed the base branch from v0.x.x to v1.x.x October 11, 2023 07:19
@daniel-zullo-frequenz
Copy link
Contributor

daniel-zullo-frequenz commented Oct 12, 2023

The commit Remove support for include_broken_batteries in control commands LGTM, I'd optionally suggest to add the reason (why) the include broken batteries feature is removed in the commit message as it is stated in the issue that this ticket is addressing (This is just a personal preference as I found it useful when tracking down changes in the git repo itself without the need to look it up in github)

shsms added 2 commits October 13, 2023 14:34
@shsms
Remove support for include_broken_batteries in control commands
bff6341 
The following items are removed:

- the `include_broken_batteries` fields in request objects and from
  function parameter lists,
- the caching of battery data, that was necessary when trying to use
  broken batteries,
- corresponding tests

This was supposed to be for handling battery data outages, but where
the batteries themselves are connected and usable. But this has
stopped being a issue, because of more stable bms firmware.

This also interferes with the safety features on the microgrid
service. So this can be removed.

Signed-off-by: Sahas Subramanian <[email protected]>
@shsms
Update RELEASE_NOTES.md
8f00cbc 
Signed-off-by: Sahas Subramanian <[email protected]>
@shsms shsms force-pushed the include-broken-batteries branch from 1b7c7ba to 8f00cbc Compare October 13, 2023 12:37
@github-actions github-actions bot removed part:core Affects the SDK core components (data structures, etc.) part:microgrid Affects the interactions with the microgrid labels Oct 13, 2023
@shsms
Copy link
Contributor Author

shsms commented Oct 13, 2023

I'd optionally suggest to add the reason (why) the include broken batteries feature is removed in the commit message

true, done

@shsms shsms marked this pull request as ready for review October 13, 2023 12:40
daniel-zullo-frequenz
daniel-zullo-frequenz reviewed Oct 13, 2023
View reviewed changes
daniel-zullo-frequenz
daniel-zullo-frequenz reviewed Oct 13, 2023
View reviewed changes
Copy link
Contributor

@daniel-zullo-frequenz daniel-zullo-frequenz left a comment

Choose a reason for hiding this comment

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

Sorry, I didn't spot it before but I think the release notes should mention include_broken_batteries instead as that is the parameter exposed to the user

@daniel-zullo-frequenz daniel-zullo-frequenz dismissed their stale review October 13, 2023 13:04

I spotted a cosmetic issue in the release notes after submitting it

@shsms @daniel-zullo-frequenz
Update RELEASE_NOTES.md
ace9102 
Co-authored-by: daniel-zullo-frequenz <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
@shsms
Copy link
Contributor Author

shsms commented Oct 13, 2023

should mention include_broken_batteries instead as that is the parameter exposed to the user

fixed

@shsms shsms added this pull request to the merge queue Oct 13, 2023
Merged via the queue into frequenz-floss:v1.x.x with commit 3c5e727 Oct 13, 2023
@shsms shsms deleted the include-broken-batteries branch October 13, 2023 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daniel-zullo-frequenz daniel-zullo-frequenz daniel-zullo-frequenz approved these changes

Assignees

No one assigned

Labels

part:actor Affects an actor ot the actors utilities (decorator, etc.) part:data-pipeline Affects the data pipeline part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove support for include_broken_batteries in the PowerDistributor

3 participants

@shsms @daniel-zullo-frequenz @llucax