Packml statistics #40
Conversation
… into kinetic-devel
There was a problem hiding this comment.
@joshuaplusone, I need your help to understand some of the structure and design decisions. Based on my understanding of the current structure, I expected the stats to come mostly from the existing state machine data. A separate class was probably appropriate, but for the most part should have been "reporting" data from the state machine (there are some exceptions, but not as many as I see in the code).
Please add an issue for test code. I believe we should address this ASAP, as it will also require testing for #37.
packml_msgs/msg/Stats.msg
Outdated
| float32 availability # executing vs faulted | ||
| float32 performance # target rate vs cycle count | ||
| float32 quality # success vs fail | ||
| float32 overall_equipment_effectiveness # perf / quality |
There was a problem hiding this comment.
This should be availability * performance * quality if I remember correctly.
packml_msgs/msg/Stats.msg
Outdated
| float32 performance | ||
| float32 quality | ||
| float32 overall_equipment_effectiveness | ||
| float32 availability # executing vs faulted |
There was a problem hiding this comment.
The comments aren't quite correct. The details of the calculations can be found here.
|
|
||
| bool transRequest(packml_msgs::Transition::Request& req, packml_msgs::Transition::Response& res); | ||
|
|
||
| private: | ||
| void handleStateChanged(packml_sm::AbstractStateMachine& state_machine, const packml_sm::StateChangedEventArgs& args); | ||
| bool resetStats(packml_msgs::ResetStats::Request& req, packml_msgs::ResetStats::Response& response); |
There was a problem hiding this comment.
So originally, the stats were envisioned as a topic with a reset service (#1). I'm not so much worried about that, but IF we want it to be a service API, perhaps we should change service type. It's a bit confusing that getStats takes a ResetStats type.
packml_sm/CMakeLists.txt
Outdated
| @@ -49,6 +50,11 @@ install(DIRECTORY include/${PROJECT_NAME}/boost/ | |||
| FILES_MATCHING PATTERN "*.h" | |||
| ) | |||
|
|
|||
| install(DIRECTORY include/${PROJECT_NAME}/ros/ | |||
There was a problem hiding this comment.
This is something I haven't seen before. I believe this will cause differences between "source" builds where packml is included in the ROS workspace vs "non-source" builds were packml is installed. Since we don't do these types of "non-source" builds, then it's probably likely we haven't ran into this yet.
| @@ -68,6 +68,8 @@ class AbstractStateMachine | |||
| double getAbortedTime(); | |||
| double getAbortingTime(); | |||
|
|
|||
| virtual void resetStats() = 0; | |||
There was a problem hiding this comment.
Is there a reason that this can't be implemented at the abstract level (maybe it will be obvious below).
| is_faulted_ = is_faulted; | ||
| } | ||
|
|
||
| void PackmlStatsProvider::setTargetRate(float target_rate) |
There was a problem hiding this comment.
What are the units of target_rate...I don't believe you captured that in header either.
| target_rate_ = target_rate; | ||
| } | ||
|
|
||
| double PackmlStatsProvider::totalDuration() const |
There was a problem hiding this comment.
Why isn't this available from the state machine?
There was a problem hiding this comment.
I will add it to the state machine.
| return 0.0f; | ||
| } | ||
|
|
||
| float PackmlStatsProvider::availabilty() const |
There was a problem hiding this comment.
Please verify availability, performance, and quality calcs. I think they are slightly wrong, based on the wiki link above. The obvious problem is that none of these should ever be greater than 1.0
|
|
||
| void PackmlStatsProvider::start() | ||
| { | ||
| start_time_ = std::chrono::steady_clock::now(); |
There was a problem hiding this comment.
Does this time need to be stored locally, or can it be extracted from the state machine?
There was a problem hiding this comment.
I am not currently tracking it in the state machine, but I will
| packml_msgs::Status status_msg_; | ||
| packml_sm::PackmlStatsProvider stats_provider_; |
There was a problem hiding this comment.
What about the stats provider requires it to be a ROS specific implementation? Shouldn't it be generic enough to capture the stats at a generic level? Shouldn't the ROS implementation just perform the conversion to ROS messages/services?
There was a problem hiding this comment.
Nothing, there is nothing ROS specific in it, other than a helper file for ROS implementations.
|
@joshuaplusone, have you created issues for testing this functionality? |
|
created issue for covering statistic gathering with unit tests here #41 |
@shaun-edwards This sets up the data structure for collecting packml stats.