Add size control system #5119
Add size control system #5119
Conversation
038facb to
db0cada
Compare
|
|
||
| public: | ||
| using IdType = Id; | ||
| using QueueTagsList = tmpl::list<QueueTags...>; |
|
|
||
| static std::string name() { return "Size"s + ::domain::name(Horizon); } | ||
|
|
||
| static std::optional<std::string> component_name( |
There was a problem hiding this comment.
Why is this an optional? I know the protocol says it is, but I can't find any documentation on what an empty return would mean.
There was a problem hiding this comment.
This is because of shape control. I added a new commit that adds some docs to the protocol.
wthrowe
left a comment
There was a problem hiding this comment.
Missed a commit in my previous review.
| namespace control_system::size { | ||
| // tt::is_a doesn't work because of domain::ObjectLabel and size_t | ||
| template <template <domain::ObjectLabel, size_t> class U, typename T> | ||
| struct is_size : std::false_type {}; |
There was a problem hiding this comment.
Do you need the first template parameter? Looks like it's always the same, and it would make is_size and is_size_v consistent.
db0cada to
bbeada2
Compare
|
Squashed in the changes and added a new commit (the most recent one) |
nilsvu
left a comment
There was a problem hiding this comment.
Just questions, don't hold up this PR
src/ControlSystem/Systems/Size.hpp
Outdated
| measurement_id.id); | ||
|
|
||
| // excision strahlkorper | ||
| Parallel::simple_action<::Actions::UpdateMessageQueue< |
There was a problem hiding this comment.
Why are there so many messages sent? Can't the data be sent in one message?
There was a problem hiding this comment.
It didn't occur to me that these were all triggering separate messages. While that's not as critical for control systems, you're right, they should be merged.
There was a problem hiding this comment.
The UpdateMessageQueue action currently only takes one queue tag at a time. This can of course be changed, but I'll do that in a separate PR.
There was a problem hiding this comment.
I was thinking just storing a tuple.
There was a problem hiding this comment.
Instead of having five separate entries in the queue, have one that stores a tagged tuple or a struct or something.
| * - Currently this control system can only be used with the \link | ||
| * control_system::ControlErrors::Size Size \endlink control error | ||
| */ | ||
| template <::domain::ObjectLabel Horizon, size_t DerivOrder> |
There was a problem hiding this comment.
How would I choose the deriv order? Maybe add docs here or to the protocol (can be another PR)
There was a problem hiding this comment.
By trial and error basically. Or know that deriv order 2 was found to work well in spec for most control systems (except a notable couple). Either way, I'll do this in another PR. I know I need to add an entire explanation on how to control system communication works anyways.
src/ControlSystem/Systems/Size.hpp
Outdated
| measurement_id.id); | ||
|
|
||
| // excision strahlkorper | ||
| Parallel::simple_action<::Actions::UpdateMessageQueue< |
There was a problem hiding this comment.
It didn't occur to me that these were all triggering separate messages. While that's not as critical for control systems, you're right, they should be merged.
bbeada2 to
1921ad8
Compare
|
@wthrowe Added new queue tags that hold TaggedTuples of the other tags. Now size only sends one message per submeasurement. |
|
Looks good. Squash. |
1921ad8 to
d7afb57
Compare
Proposed changes
Also replace some interpolated quantities in the CharSpeed measurement. The current ones are no longer necessary. And some other small miscellaneous changes.
Upgrade instructions
Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.Further comments