-
Notifications
You must be signed in to change notification settings - Fork 0
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
WLM control by task handler #27
Conversation
self._message_queue: MessageQueue = settings.MESSAGE_QUEUE | ||
self._measure_queue: MeasureQueue = MeasureQueue() | ||
self._channel_to_period: dict[int, timedelta] = {} | ||
self._open_connection() | ||
|
||
def _open_connection(self): |
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.
This method may cause several errors, e.g., there is no Config
, failed to open WLM.
In fact, regarding WLM, it is always possible to fail the API call as the USB connection goes wrong sometimes.
I think we should carefully handle such error cases.
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.
Maybe modeling this handler as an FSM would be helpful..?
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.
Umm.. Indeed, I didn't consider any error on purpose.
To send the occurred error info to the request handler, we need to implement more APIs, hence I'd like to handle this after implementing main features.
What do you think about this? @kangz12345
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.
Alright then, for now the structure might vary in the future, so let's handle the errors later.
self._wlm.close() | ||
|
||
def _start_channel_measurement(self, channel: int): | ||
setting = Setting.objects.filter(channel__name=channel).order_by('-created_at').first() |
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.
Let's keep this in mind: the filtering might cost quite a lot in the future.
For now I think it would be fine.
Co-authored-by: Jiyong Kang <[email protected]> See also: #6
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.
LGTM
This resolves #6.
I couldn't test these features since it needs a WLM device, so please check the codes carefully.