Add previously unfinished job manager #20
Conversation
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
There was a problem hiding this comment.
In general I think the cleanup is good. Personally I'm not a fan of APIs throwing runtime errors and having to wrap stuff in try-catch blocks. I would prefer if the APIs could rely on returning nullopts or other boolean values indicating unsuccessful executions.
| }); | ||
| if (it != this->_jobs.end()) | ||
| { | ||
| throw JobError("Another task with the same id already exist"); |
There was a problem hiding this comment.
Again to move away from the catch-try pattern, can we return optional<JobPtr> with std::nullopt in this case?
There was a problem hiding this comment.
I don't mind banning exceptions, but exceptions are used in many places, I think we should do a large scale refactor if that is the direction. Also can you point me to the style guide explaining the use of exceptions? Also would be good to have a link to the style guide in a DEVELOPERS file.
Instead of std::optional, I propose to use std::variant instead. std::optional does not allow the reason for failure to be reported. std::variant would be closest to the way rust does error handling, an alternative is to use std::pair, which would be closer to go.
| "]: Behavior tree returned invalid or unknown status [" << | ||
| bt_status << "]"; | ||
| RCLCPP_ERROR_STREAM(job.ctx->node->get_logger(), oss.str()); | ||
| throw std::runtime_error(oss.str()); |
There was a problem hiding this comment.
Thinking about the design decision to throw an exception within JobManager and rely on the workcell orchestrator to catch it handle the error accordingly. Why not directly halt all jobs within the JobManager and not rely on a try-catch block?
There was a problem hiding this comment.
The reason is because WorkcellOrchestrator may need to know when an error happens (can't rmb if we actually need it currently but having that option is good). May be a good idea to halt + throw exception, I think either is ok, having workcell orchestrator handle the error does make it easier to handle edge cases.
| RCLCPP_INFO(this->get_logger(), "Cleaned up"); | ||
| return CallbackReturn::SUCCESS; | ||
| } | ||
|
|
||
| auto WorkcellOrchestrator::_configure( | ||
| const rclcpp_lifecycle::State& /* previous_state */) -> CallbackReturn | ||
| { | ||
| this->_ctx_mgr = std::make_shared<ContextManager>(); | ||
| this->_job_mgr = JobManager(this->shared_from_this(), 1); |
There was a problem hiding this comment.
Why 1 for max_concurrent? Didn't we have a ROS parameter to set this number?
There was a problem hiding this comment.
We don't want a workcell to be able to execute multiple tasks at once so it is hardcoded. The option is there so that system orchestrator can reuse the code in the future.
| @@ -214,7 +214,7 @@ auto WorkcellOrchestrator::on_activate( | |||
| RCLCPP_INFO(this->get_logger(), "Workcell activated"); | |||
| this->_bt_timer = this->create_wall_timer(BT_TICK_RATE, [this]() | |||
| { | |||
| this->_tick_all_bts(); | |||
| this->_job_mgr->tick(); | |||
There was a problem hiding this comment.
Should we check whether _job_mgr.has_value() before ticking? And same within on_deactive, on_cleanup, etc.
There was a problem hiding this comment.
Using this->_job_mgr.value().tick(); instead, if there is no value it will throw an exception.
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
| private: std::unique_ptr<lifecycle_manager::LifecycleManager<>> _lifecycle_mgr{ | ||
| nullptr}; | ||
| private: std::filesystem::path _bt_path; | ||
| // mapping of mapped task type and the original | ||
| private: std::unordered_map<std::string, std::string> _task_remaps; | ||
| private: TaskParser _task_parser; | ||
| private: std::optional<JobManager> _job_mgr; |
There was a problem hiding this comment.
I can't say I really follow all the different types used for member variables, from what I understand:
- Normal objects (i.e.
TaskParser _task_parser;) for objects that can be default initialized. shared_ptrfor objects that can't be initialized in a constructor and are shared with other instances (i.e. theLifecycleNode).unique_ptrfor objects that can't be initialized in a constructor and don't need to be shared (i.e.std::unique_ptr<lifecycle_manager::LifecycleManager<>> _lifecycle_mgr{ nullptr};)std::optionalintroduced here for... An objects that doesn't need to be shared and can't be initialized in a constructor?
It seems to me there is overlap with what can be achieved with std::unique_ptr and std::optional, but using both at the same time is a bit confusing.
This completes the previous work left halfway https://github.com/osrf/nexus/blob/main/nexus_workcell_orchestrator/src/job.cpp.
It introduces a
JobManagerto help keep track of tasks, there should be no functionalities differences with the existing code, but it does make unit testing much easier and makes it easier to maintain in the future.