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

Expander boot handling with error codes #118

Open
wants to merge 52 commits into
base: main
Choose a base branch
from

Conversation

JensOgorek
Copy link
Contributor

This is a possible implementation of error codes on the expander module.

@JensOgorek JensOgorek added the enhancement New feature or request label Dec 13, 2024
@JensOgorek JensOgorek linked an issue Dec 13, 2024 that may be closed by this pull request
14 tasks
@JensOgorek
Copy link
Contributor Author

@falkoschindler I moved the error handling to its own pull request

Base automatically changed from expander_module_startup_handling to main January 8, 2025 10:45
Copy link
Collaborator

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

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

I have some questions and doubts about the current concept:

  1. Are error codes meant to be binary flags that can be added to get combined errors like, e.g., 5 = 1 (low voltage) + 4 (no response from motor driver)? If so, set_error would overwrite existing errors when setting another one.
  2. Currently, set_error can be called with ERROR_NONE, which results in a wrong value for has_error_. It looks like the ErrorHandling API would need additional methods like add_error, remove_error and clear_errors to cover all cases. But then it might be easier to just expose the error_codes map directly.
  3. Is it a good idea to share a common list of error codes across all modules? I can imagine conflicts when a module needs an error similar but not identical to one from another module. We would either need to introduce many error codes with similar names and meanings, or pricisely document error codes per module. If so, I don't see the point in a common list of error codes.

Maybe the current implementation with only two errors in a single module is too early to see the big picture. But at the moment I lean towards a new "error" property in the expander module with two additive codes (connection timeout and connection loss). This way it can be accessed like any other property, used in rules, and added to the core output line.

If we really want a global error property or method in the core module, we can iterate over modules and aggregate errors. But right now there is no benefit.

@JensOgorek
Copy link
Contributor Author

Branch is restored and diff should be correct now.

@JensOgorek
Copy link
Contributor Author

JensOgorek commented Jan 8, 2025

  1. Are error codes meant to be binary flags that can be added to get combined errors like, e.g., 5 = 1 (low voltage) + 4 (no response from motor driver)? If so, set_error would overwrite existing errors when setting another one.

I thought about it, and using a bit mask could solve this. Each error would be a single bit, allowing multiple errors to be combined without overwriting.

  1. Currently, set_error can be called with ERROR_NONE, which results in a wrong value for has_error_. It looks like the ErrorHandling API would need additional methods like add_error, remove_error and clear_errors to cover all cases. But then it might be easier to just expose the error_codes map directly.

With a bit mask, ERROR_NONE is unnecessary, since a value of 0 naturally represents no errors. However, I think we would still need these API methods (add_error, remove_error, clear_errors) to handle the bit mask more easily and efficiently.

  1. Is it a good idea to share a common list of error codes across all modules? I can imagine conflicts when a module needs an error similar but not identical to one from another module. We would either need to introduce many error codes with similar names and meanings, or pricisely document error codes per module. If so, I don't see the point in a common list of error codes.

I think some error codes could be in all modules (TIME_OUT), but having individual error code lists for the modules gives more flexibility and avoids conflicts between similar errors.

@JensOgorek JensOgorek self-assigned this Jan 10, 2025
@JensOgorek JensOgorek added this to the 0.7.0 milestone Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve expander module handling during startup
2 participants