Skip to content
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

Zones-meta and Notification updates #664

Open
wants to merge 46 commits into
base: master
Choose a base branch
from

Conversation

godind
Copy link

@godind godind commented Apr 26, 2024

Update to change ffrom alarm to notification. Adjusted zones and operationnal concept.

| emergency | The value indicates a life-threatening condition |

## Resolving and silencing notifications
* To silence a notification, the serve or clients, send delta notification replica without the `sound` item the method array.
Copy link
Member

@panaaj panaaj Apr 27, 2024

Choose a reason for hiding this comment

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

Actioning a notification is an operation of the notification manager.
As this applies to implementation, the expected operation should be outlined here.

Copy link
Author

Choose a reason for hiding this comment

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

I thought about this yesterday, I was including Alarm Clearer plugin as a standard setup in this writing. What do you recommend?

Copy link
Author

Choose a reason for hiding this comment

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

It would be nice to be able to explain how consumers should work the system.

@sbender9
Copy link
Member

Can you write a larger summary of the changes? Really hard to parse and make sense of the diffs.

@tkurki
Copy link
Member

tkurki commented Apr 27, 2024

Diff ignoring whitespace changes is better https://github.com/SignalK/specification/pull/664/files?w=1

@godind
Copy link
Author

godind commented Apr 28, 2024

There was no squash commit for some unknown reason.

Maybe read from my fork here:
https://github.com/godind/specification/tree/Zones-meta/mdbook/src

Not sure how else to do it but pasting the whole thing here. Or I could create another branch with a single commit.

@tkurki
Copy link
Member

tkurki commented Apr 28, 2024

The PR is based on that branch so there is no difference whether you look at the PR or the branch.

If you don't squash then there is no squash, just your commits.

But in terms of understanding what has changed squashing or the long list of commits with less than understandable commit messages is not the problem. You have made so many changes to line breaks without changing the content that it is needlessly hard to understand what are the meaningful changes. This is what Scott is asking - the PR description is pretty cryptic. So please can you summarise how and why this PR changes things?

@tkurki
Copy link
Member

tkurki commented Apr 28, 2024

@godind
Copy link
Author

godind commented May 28, 2024

What do you suggest to fix this? I'm not sure how I can resubmit in one change without line breaks. I formatted so I could read myself.

It's a learning curve.

Thanks

@tkurki
Copy link
Member

tkurki commented Jun 2, 2024

So please can you summarise how and why this PR changes things?

@godind
Copy link
Author

godind commented Jun 3, 2024

Notification changes

  • Reformulates the base conceptual approach from Alarm to Notification and expresses that alarms are notification severity state.
  • Defines the Normal state as the default data state when path values matches no other zones.
  • Changes the approach to clear a Notification from the prior spec which was to send an update the notification path with:
    "values": [ { "path": "notifications.mob", "value": null } ]
    to sending an update the notification path with normal state and an empty method:
    "value": { "message": "", "state": "normal", "method": [] }
  • Adds the concept of Silencing and Resolving Notifications: To silence, update notification by removing sound from the method array values. Resolve, update notification by setting state to normal.
  • Copied Alarm Management section from Metadata page to this page and renamed to Notification Management as I thought it made more sense in the page.
  • Fixed link to metadata page.

Metadata Changes

  • Focused on zones reaffirming the base conceptual approach from Alarm to Notification and expresses that an alarm is a notification severity state.
  • Also reaffirms normal as the state where no zones match the value.
  • Added zones configuration exemples to showing where zones state could repeat themselves for different value ranges.
  • Removed Alarm Management section. I thought it should go in Notifications page as it's about notification management process more than about meta config settings.

Please read the output as this is my first spec update. I wrote this based on my exchange with Scott and what I understood. I'm not without fault...

I hope what I wrote helps!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants