Skip to content

Conversation

@RenaultR42
Copy link
Contributor

Issue: #12

elif msg['type'] == "PRES":
if ((msg['val'] > proto.settings_values['PK'] + proto.settings_values['ADPK']) or
(msg['val'] < proto.settings_values['PK'] - proto.settings_values['ADPK'])):
self.request_queue.put({'type': 'alarm', 'priority': 42, 'value': 5})
Copy link
Member

Choose a reason for hiding this comment

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

how is the priority used? Does this make sense given that priority queues are not available for multiprocessing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my opinion the priority can have a sense. But it depends on what is the final design around alarm. We can imagine a design where a alarm code has its own sound or visual and if several alarms are triggered at the same time, the higher priority can be played first, and when it is fixed the second one, etc.
At least it is easy to remove.

For the value, yes it is just for test, ideally we should assign a name to a value and having a list of alarms somewhere (in protocol.py ?). I made the pull request to get a feedback and not really to merge it yet, just to define a bit more what is expected and if the concept is right.

elif msg['type'] == "PRES":
if ((msg['val'] > proto.settings_values['PK'] + proto.settings_values['ADPK']) or
(msg['val'] < proto.settings_values['PK'] - proto.settings_values['ADPK'])):
self.request_queue.put({'type': 'alarm', 'priority': 42, 'value': 5})
Copy link
Member

Choose a reason for hiding this comment

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

5 seems a strange value if we're still using bit field alarms. Ideally we'd have a list of the definitions available somewhere. Is this a placeholder?

Copy link
Member

@TomBruyneel TomBruyneel left a comment

Choose a reason for hiding this comment

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

Not complete yet, it's a bit more complicated than this. It's only the peak value that has to be in the threshold. So you need to do a function analysis on the last N points to achieve this. Align with @Merwanski on this problem, he started working on the thread needed to do this here: #22

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.

4 participants