-
Notifications
You must be signed in to change notification settings - Fork 4
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
Feature trigger class definition #90
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good. Only problem I had was that the entire module expect c++11
to be used. This caused the compiler to fail the building of the code. I suggest that you add set (CMAKE_CXX_STANDARD 11)
to the CMakeLists.txt
file. Just add it here
https://github.com/RI-SE/Maestro/blob/b6c42401153f52590f39417f1a655516b9ce7a7c/modules/ScenarioControl/CMakeLists.txt#L6
I also have some comment about the documentation of the class, might be worth to just check them out to see what you think.
|
||
private: | ||
TriggerID_t triggerID; | ||
std::vector<TriggerParameter_t> parameters; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it hard to understand what this is and what is intended to be used for. Is the list a general description of a trigger on a high level? If this is the case is there an example of an intended use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have some opinions on this. I intended it to store the ISO parameters (e.g. ACCELERATION, GREATER_THAN, VALUE) for parsing into an operation mode for the trigger. Perhaps it is not interesting to conform to ISO here? Currently, it is used by e.g. BooleanTrigger to select between flank triggering and high/low etc. . But eventually we will read this from a file...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good. Seems like the points raised by Viktor have been addressed and I have noting new to add.
I've defined three classes here:
My thinking is that another type can be defined for acceleration which naturally inherits from yet another type handling e.g. floating point signals.