Allow extensibility of BCP-008 status monitors - #511
Merged
Conversation
jonathan-r-thorpe
commented
Jul 27, 2026
jonathan-r-thorpe
commented
Contributor
- Refactor BCP-008 implementation to generalize the control protocol's handling of monitor domains.
- Allows extensibility of existing status monitor definitions
- Allows users to define their own status monitors
- Aligns the code more closely with the BCP-008 specification
- No change to external facing API
…andling of monitor domains.
…ructuring the initialization for better clarity and maintainability.
jonathan-r-thorpe
marked this pull request as draft
July 27, 2026 19:54
jonathan-r-thorpe
marked this pull request as ready for review
July 28, 2026 09:18
lo-simon
approved these changes
Aug 3, 2026
lo-simon
left a comment
lo-simon
left a comment
Collaborator
There was a problem hiding this comment.
Some minor suggestions.
Collaborator
There was a problem hiding this comment.
I think all the direct calls to the set_monitor_status functions should be hidden from the user. The user should only be using those delay functions set_monitor_status_with_delay. Maybe we should rename all those delay functions too.
| bool erase(const utility::string_t& datatype_name); | ||
|
|
||
| // insert monitor domains for the given class id, false if a profile already exists | ||
| bool insert(const nc_class_id& class_id, const std::vector<monitor_domain>& monitor_domains); |
Collaborator
There was a problem hiding this comment.
Shall the insert function be renamed to insert_monitor_domains to match the erase_monitor_domains function?
…_protocol_state class; update corresponding header and test cases to reflect the new method name.
lo-simon
approved these changes
Aug 5, 2026
| bool is_deprecated = false); | ||
| } | ||
|
|
||
| typedef std::function<std::vector<experimental::monitor_domain>(const nc_class_id& class_id)> get_monitor_domains_handler; |
Collaborator
There was a problem hiding this comment.
Would it be better to move the get_monitor_domains_handler into control_protocol_handlers.
See move-get_monitor_domains_handler.patch
Contributor
Author
There was a problem hiding this comment.
Moved to control_protocol_handlers
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.