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.

Sign up for GitHub

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

Jump to bottom

Report broken i2c status in front-end #1391

Draft

amrsoll wants to merge 1 commit into develop from SW-82/bugfix/show-i2c-broken-err

Contributor

amrsoll commented Nov 17, 2021 •

edited

Loading

Major:

Add a new Error message in the front end: i2c broken
Only show 1 error at a time - the one with highest priority
Fan handler does not time out as long as some data comes regularly

Refactor:

docstrings
var = foo[bar] => foo.get(bar, var) # when bar is not a mandatory key
Add comments in some obscure code places
foo == False => not foo
magic number => constant
mutate constant => copy constant as private var
Simplify Threads (un-nesting)
except: => except Exception: (does not catch SIGTERM)
Remove superflous try / except
logger.error(my_exception) => logger.exception(my_exception)
def foo(): pass => foo = do_nothing_func
- saves lines
- highlights that foo() is not important
complex if / elif key in foo: bar(foo[key]) => use lookup map
- better readability
- saves a bunch of lines (see _handle_fan_dynamic)
foo.get("bar", None) == "barbar" => foo.get("bar") == "barbar"
Repeated foo[bar] => var = foo[bar]
Avoid basic classes as var names (str)
Remove unused functions

Minor :

Handles partial fan dynamic datasets as an update (refresh) for the dynamic dataset

Known bug: If iobeam reports partial operational data (new default), the laser temperature gets "out of date" and stops the laser job

amrsoll added bug iobeam UI/UX labels

amrsoll force-pushed the SW-82/bugfix/show-i2c-broken-err branch from b54e4bd to 441b61b Compare

November 17, 2021 14:05

amrsoll commented

View reviewed changes

octoprint_mrbeam/printing/acc_line_buffer.py

Comment on lines -96 to -107

-                  def get_first_item(self):
-                      """
-                      Returns the first (oldest) item. This is the one to be removed next.
-                      :return: item dict(cmd="", i=1, f=23) or None if empty
-                      """
-                      if self.is_empty():
-                          return None
-                      self._lock.reader_acquire()
-                      res = self.buffer_cmds[0]
-                      self._lock.reader_release()
-                      return res

Contributor Author

amrsoll Nov 17, 2021

unused function

amrsoll commented

View reviewed changes

octoprint_mrbeam/iobeam/hw_malfunction_handler.py

@@ @@ -91,7 +91,16 @@ def show_hw_malfunction_notification(self): @@
                       )
                       for malfunction_id, data in messages_sorted:
-                          if malfunction_id == self.MALFUNCTION_ID_BOTTOM_OPEN:
+                          if malfunction_id == "i2c_bus_malfunction":

Contributor Author

amrsoll Nov 17, 2021

Constant?

amrsoll commented

View reviewed changes

octoprint_mrbeam/iobeam/iobeam_handler.py

Comment on lines +168 to +169

		self.socket_file = plugin._settings.get(["dev", "sockets", "iobeam"]) or socket_file
		self._my_socket = None # The actual socket object

Contributor Author

amrsoll Nov 17, 2021

TODO : externalise settings management to the MrBeamPlugin class

amrsoll commented

View reviewed changes

octoprint_mrbeam/iobeam/iobeam_handler.py

Comment on lines -202 to +207

-                      # We only start the iobeam listener now
-                      iobeam_worker = threading.Timer(1.0, self._initWorker, [self._socket_file])
-                      iobeam_worker.daemon = True
-                      iobeam_worker.start()
+                      self._worker = threading.Thread(target=self._work, name="iobeamHandler")
+                      self._worker.daemon = True
+                      self._worker.start()

Contributor Author

amrsoll Nov 17, 2021

starting the iobeam worker does not have a consequence as it will be waiting for the socket file to show up.

amrsoll commented

View reviewed changes

octoprint_mrbeam/iobeam/iobeam_handler.py

Comment on lines -228 to -236

-                  def send_temperature_request(self):
-                      """
-                      Request a single temperature value from iobeam.
-                      :return: True if the command was sent sucessfully.
-                      """
-                      return self._send_command(
-                          self.get_request_msg([self.MESSAGE_DEVICE_LASER + "_temp"])
-                      )

Contributor Author

amrsoll Nov 17, 2021

unused

amrsoll commented

View reviewed changes

octoprint_mrbeam/iobeam/iobeam_handler.py

Comment on lines -421 to -432

-                  def _initWorker(self, socket_file=None):
-                      self._logger.debug("initializing worker thread")
-                      # this is needed for unit tests
-                      if socket_file is not None:
-                          self.SOCKET_FILE = socket_file
-                      # this si executed on a TimerThread. let's start a plain thread, just to have it "clean"
-                      self._worker = threading.Thread(target=self._work, name="iobeamHandler")
-                      self._worker.daemon = True
-                      self._worker.start()

Contributor Author

amrsoll Nov 17, 2021

Complicates the spawning of threads

amrsoll commented

View reviewed changes

octoprint_mrbeam/iobeam/iobeam_handler.py

Comment on lines +787 to 789

+                          self._last_i2c_monitoring_dataset = dataset
+                          return 1
                       self._last_i2c_monitoring_dataset = dataset

Contributor Author

amrsoll Nov 17, 2021 •

edited

Loading

monitoring dataset was not properly updated when there is an error

amrsoll commented

View reviewed changes

octoprint_mrbeam/iobeam/iobeam_handler.py

@@ @@ -1122,21 +1056,15 @@ def _handle_error_message(self, message): @@
                           raise Exception("ioBeam requested to reconnect. Now doing so...")
                       return 1
-                  def _handle_response(self, message):
+                  def _handle_response(self, response):

Contributor Author

amrsoll Nov 17, 2021

separation of concern: The response handler no longer needs to know the whole context of the "response" submap

amrsoll commented

View reviewed changes

octoprint_mrbeam/iobeam/iobeam_handler.py

+                          >>> io_handler.get_request_msg(["fan"])
+                          {"request": [fan"], request_id": 0}
+                          >>> io_handler.get_request_msg(["fan", "laser"])
+                          {"request": [fan", "laser"], "request_id": 1, "value": 70}

Contributor Author

amrsoll Nov 17, 2021 •

edited

Loading

FIXME: should not have a "value" key

ahmed-mrbeam force-pushed the develop branch from fa2a370 to 1798842 Compare

November 17, 2021 14:44


          Report broken i2c status in front-end

a6b748e

Major:

- Add a new Error message in the front end: i2c broken
- Only show 1 error at a time - the one with highest priority
- Fan handler does not time out as long as some data comes regularly
-

Refactor:

- docstrings
- var = foo[bar] => foo.get(bar, var) # when bar is not a mandatory key
- Add comments in some obscure code places
- foo == False => not foo
- magic number => constant
- mutate constant => copy constant as private var
- Simplify Threads (un-nesting)
- except: => except Exception: (does not catch SIGTERM)
- Remove superflous try / except
- logger.error(my_exception) => logger.exception(my_exception)
- def foo(): pass => foo = do_nothing_func
  - saves lines
  - highlights that foo() is not important
- complex if / elif key in foo: bar(foo[key]) => use lookup map
  - better readability
  - saves a bunch of lines (see _handle_fan_dynamic)
- foo.get("bar", None) == "barbar" => foo.get("bar") == "barbar"
- Repeated foo[bar] => var = foo[bar]
- Avoid basic classes as var names (str)
- Remove unused functions

Minor :

- Handles partial fan dynamic datasets as an update (refresh) for the dynamic dataset

Known bug: If iobeam reports partial operational data (new default), the laser temperature gets "out of date" and stops the laser job

amrsoll force-pushed the SW-82/bugfix/show-i2c-broken-err branch from 441b61b to a6b748e Compare

November 17, 2021 15:38

ahmed-mrbeam force-pushed the develop branch 2 times, most recently from 1f28493 to 6c61b35 Compare

December 8, 2021 15:55

ahmed-mrbeam assigned ahmed-mrbeam and Josef-MrBeam and unassigned ahmed-mrbeam

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug iobeam UI/UX