Time to reach rework #1634
Time to reach rework #1634
Conversation
knoellle
added
the
is:Refactoring
knoellle
force-pushed
the
time-to-reach-rework
branch
from
de2b39b to
570ca6c
Compare
| stand_up_back_estimated_remaining_duration: | ||
| CyclerState<Duration, "stand_up_back_estimated_remaining_duration">, |
There was a problem hiding this comment.
Did you think about this use of CyclerState twice? We should be very sure about behavior at edges. e.g. the duration being ZERO when the standup never happened yet? etc.
But there is definitely something incorrect in this module
There was a problem hiding this comment.
The incorrectness should be fixed™️ now.
The cycler state is now always set to 0 unless the robot is currently performing this motion, in which case it uses the remaining duration from the iterator. The default case of the duration being 0 is perfectly fine since it is only used in an addition.
I need to use a cycler state here because this replaces the time to reach cycler state. We have a loop for certain and I think it makes sense to make the cut for unrolling it in this place.
There was a problem hiding this comment.
Okay, I agree in parts. Maybe we should discuss this in person to reduce the chance for miscommunication.
I currently struggle a little to have the remaining duration set to 0 in the default case. This somehow mixes information of the actual time the interpolation will take to finish with the motion selection of actually standing up... and both do the cycle-loop which you cut here... Maybe that's correct... 🤔
|
|
||
| let team_ball = self.get_best_received_ball( | ||
| context.cycle_time.start_time, | ||
| context.striker_trusts_team_ball.mul_f32(4.5), |
There was a problem hiding this comment.
Should this be dependent on this other parameter?
knoellle
force-pushed
the
time-to-reach-rework
branch
3 times, most recently
from
d980b47 to
b4bd753
Compare
knoellle
force-pushed
the
time-to-reach-rework
branch
from
b4bd753 to
e197cb4
Compare
| #[derive(Default)] | ||
| pub struct MainOutputs { | ||
| pub team_ball: MainOutput<Option<BallPosition<Field>>>, | ||
| pub network_robot_obstacles: MainOutput<Vec<Point2<Ground>>>, |
There was a problem hiding this comment.
I'm fine with the current solution, but this may also be moved to a completely independent separate Node...
What do you think?
There was a problem hiding this comment.
Yes, was planning on doing that; will do.
| // Ignore everything during penalty_* | ||
| if let Some(game_controller_state) = context.filtered_game_controller_state { | ||
| let in_penalty_shootout = matches!( | ||
| game_controller_state.game_phase, | ||
| GamePhase::PenaltyShootout { .. } | ||
| ); | ||
| let in_penalty_kick = game_controller_state.sub_state == Some(SubState::PenaltyKick); | ||
|
|
||
| if in_penalty_shootout || in_penalty_kick { | ||
| return Ok(MainOutputs { | ||
| team_ball: None.into(), | ||
| network_robot_obstacles: Vec::new().into(), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
This can happen way earlier at the beginning of the function, can it?
There was a problem hiding this comment.
Yes, but I'd like to keep updating the internal state during these times. Not important for penalty shootout, but maybe remembering that someone saw a ball a few seconds ago when a penalty kick ends helps.
There was a problem hiding this comment.
maybe this gets clearer, if we chop the cycle method into even smaller functions, just to improve readability, to easier realize, that you intentionally first update internal state and then return for penalty shootout.
This need is also visible as you wrote comments for structuring the function. These should probably better be functions with descriptive names.
something like
fn cycle(...) {
self.do_foo();
let stuff = self.collect_stuff();
self.update_state(stuff);
if penalty_shootout {
return None;
}
self.do_bar();
return Some;
}
| role_assignment[PlayerNumber::Seven] = Some(optional_role); | ||
| } | ||
| for (player_number, &optional_role) in unassigned_players.into_iter().zip(optional_roles) { | ||
| role_assignment[player_number] = Some(optional_role) | ||
| } | ||
|
|
||
| role_assignment[own_player_number].unwrap_or_default() |
There was a problem hiding this comment.
Can we remove Default from Role entirely? I do not like to have Default for a role, as this depends on the context. There is no general, natural default value to a role. Being a striker is a implementation in some fallback cases of the role assignment.
The consequence of that is to explicitly state that we want to assign striker(s) in respective cases
| self.last_system_time_transmitted_game_controller_return_message | ||
| .unwrap(), | ||
| )? > context.spl_network.game_controller_return_message_interval; | ||
| self.try_sending_game_controller_return_message(&context, ground_to_field)?; |
There was a problem hiding this comment.
I do not know how to add comments to lines you did not touch, so leaving it here, as this place is related...
I am not sure if setting a default/fallback for the ground_to_field for the entire node. I think initially we added this fallback to send game controller return messages with an initial pose even though localization does not provide such a pose.
Having a default (in this case probably 0,0,0) for all cases where localization decides to not know about the robot's location, opens the door for bugs in role assignment. If we somehow require a ground_to_field in role assignment, maybe even for deciding if we become striker, and localization provides None, this is definitely a bug and should not stay hidden by just using (0,0,0), which actually can be much worse.
| || cycle_start_time | ||
| .duration_since(self.last_transmitted_spl_striker_message.unwrap())? | ||
| > context.spl_network.spl_striker_message_send_interval; | ||
| // TODO: reimplement whatever this did |
| @@ -35,10 +39,9 @@ use crate::localization::generate_initial_pose; | |||
| pub struct RoleAssignment { | |||
| last_received_spl_striker_message: Option<SystemTime>, | |||
There was a problem hiding this comment.
This can never be None, can it? This is only None in construction and the first cycle immediately sets a Some to it because self.role_initialized == false
|
|
||
| let spl_striker_message_timeout = match self.last_received_spl_striker_message { | ||
| None => false, | ||
| None => true, | ||
| Some(last_received_spl_striker_message) => { | ||
| cycle_start_time.duration_since(last_received_spl_striker_message)? | ||
| > context.spl_network.spl_striker_message_receive_timeout |
There was a problem hiding this comment.
This is a horrible name... I'm trying to wrap my head around what it actually is for minutes now. If I'd only stumble upon that in the parameters, I'd have even less of an idea what this controls
There was a problem hiding this comment.
This is something like the duration we believe in the last information a striker sent us before we assume the robot which sent us the information is dead and we have to fallback and continue somehow, is it?
There was a problem hiding this comment.
so it is a duration_this_robot_believes_in_the_last_information_of_another_striker_before_assuming_it_fell_dead_and_we_have_to_continue_doing_something_else
@h3ndrk would be happy 😆
| Role::Striker => { | ||
| send_spl_striker_message = true; | ||
| team_ball = None; | ||
| new_role = Role::Loser; |
There was a problem hiding this comment.
Same as above, being incapable of commenting some random unchanged lines...
This entire pattern of having a mutable new_role makes the entire state machine and role assignment (i.e. the actual assignment of the role) depending on the control flow and order of the code.
What I want to say is, if I later (maintaining this role assignment node) want to add some functionality I'd have to be extremely careful where to put it in the mutable path of new_role. Employing a functional/immutable style of assigning this value would not have this flaw/risk.
But this may also be an even bigger refactoring... The current structure of the entire logic seems to be inherently based on that mutable path of multiple functions incrementally editing the new_role. What do you think?
| sub_state: Some(SubState::PenaltyKick), | ||
| .. | ||
| }) | ||
| ); | ||
| if spl_striker_message_timeout && !is_in_penalty_kick { |
There was a problem hiding this comment.
Don't we have to first handle incoming messages before we decide to distrust a striker because of spl_striker_message_timeout?
I'm just thinking of the order in time of things. Incoming messages may be in the past, and the fact that the striker timeouted is happening now, so definitely after all possible incoming messages.
In my head, first for example becoming a searcher in the new_role changes things for the incoming messages afterwards...
| Role::ReplacementKeeper => { | ||
| team_ball = None; | ||
| } | ||
| Role::Keeper | Role::ReplacementKeeper => {} |
There was a problem hiding this comment.
We should discuss the meaning/reason of this match statement and its implication. How can a striker message timeout and this very own robot be striker?!?! ... to then become loser... I'm confused
| @@ -355,7 +269,6 @@ impl RoleAssignment { | |||
| } else { | |||
| self.role = new_role; | |||
| } | |||
| self.team_ball = team_ball; | |||
|
|
|||
| if let Some(game_controller_state) = context.filtered_game_controller_state { | |||
There was a problem hiding this comment.
Is it correct that last_time_player_was_penalized is updated after it is used in this cycle?
| #[derive(Clone, Copy, Debug)] | ||
| enum Event { | ||
| None, | ||
| Striker(StrikerEvent), |
There was a problem hiding this comment.
what about making this a structured enum variant?
There was a problem hiding this comment.
The inner type is also used as a function argument.
| if primary_state != PrimaryState::Playing { | ||
| match detected_own_ball { | ||
| None => return (current_role, false, team_ball), | ||
| Some(..) => { | ||
| return ( | ||
| current_role, | ||
| false, | ||
| team_ball_from_seen_ball(detected_own_ball, current_pose, cycle_start_time), | ||
| ) | ||
| } | ||
| } | ||
| return current_role; | ||
| } |
There was a problem hiding this comment.
why do we trigger the state machine during all the other states? i'd expected role assignment to return early in the current way we handle ready and set...
TLDR: why is this here in the state machine and not earlier in the cycle method
| filtered_game_controller_state, | ||
| optional_roles, | ||
| ), | ||
| match (current_role, detected_own_ball, event) { |
There was a problem hiding this comment.
can we make the detected_own_ball an enum? this would make reading the match arms much easier
| ), | ||
| match (current_role, detected_own_ball, event) { | ||
| // Striker lost Ball | ||
| (Role::Striker, false, Event::None | Event::Loser) => match team_ball { |
There was a problem hiding this comment.
| (Role::Striker, false, Event::None | Event::Loser) => match team_ball { | |
| (Role::Striker, false, Event::Tick | Event::Loser) => match team_ball { |
just naming that came to mind. what do you think?
| filtered_game_controller_state, | ||
| optional_roles, | ||
| ), | ||
| // Striker remains Striker, sends message after timeout |
There was a problem hiding this comment.
why this comment part about "sends message after timeout"?
| // TODO: On main, this sends a striker message immediately, ignoring the spl_striker_message_send_interval | ||
| // but not the silence_interval_between_messages. | ||
| // With the new implementation this is no longer possible since the message sending | ||
| // is only based on the previous and new roles and does not consider the cause of the | ||
| // transition. | ||
| // This was already broken on main since the message would silently be dropped if the | ||
| // edge case occured within the silence interval but might lead to a faster | ||
| // striker convergence in this rare circumstance. |
There was a problem hiding this comment.
marking this with a comment for discussion...
| (Role::Striker, true, Event::Loser) => Role::Striker, | ||
|
|
||
| // Loser remains Loser | ||
| (Role::Loser, false, Event::None) => Role::Loser, |
There was a problem hiding this comment.
We could use the nested values for this local scope. allowing to write
| (Role::Loser, false, Event::None) => Role::Loser, | |
| (Loser, false, None) => Role::Loser, |
the current naming would produce a weird conflict between Event and Role (Striker and Striker), where i'd suggest renaming the events. maybe StrikerMessage
what do you think? the current formulation is also not annoyingly long. so i'd also be fine keeping it as is
| let time_to_reach_viable = | ||
| time_to_reach_kick_position.is_some_and(|duration| duration < Duration::from_secs(1200)); |
| @@ -667,70 +611,26 @@ fn seen_ball_to_game_controller_ball_position( | |||
| }) | |||
| } | |||
|
|
|||
| fn seen_ball_to_hulks_network_ball_position( | |||
| ball: Option<&BallPosition<Ground>>, | |||
| fn own_ball_to_hulks_network_ball_position( | |||
There was a problem hiding this comment.
what about making this and the other similar conversion functions methods of one of the types?
Why? What?
This PR refactors
I still haven't been able to reproduce the exact bug related to time to reach that we found at RoboCup last year but this PR should hopefully fix it.
Fixes #
ToDo / Known Issues
There are still some todos left in the code
Ideas for Next Iterations (Not This PR)
How to Test
Best approach is probably to play around with robots, both in simulation and on real hardware.