Refactor SensorState to have a single set of sensors, and use states #10
Conversation
Avoid racing to update sensor state to connected.
| .bt_session | ||
| .connect(&sensor.id) | ||
| .await | ||
| .with_context(|| std::line!().to_string())?; |
There was a problem hiding this comment.
Should the connection_status be updated if this fails, rather than leaving it in the Connecting state?
There was a problem hiding this comment.
I think this is now unnecessary because the state will revert to what it was before it was updated to Connecting. Now I think the only reason the Connecting state has an effect is if an update for the sensor comes in before this function returns. We can probably solve this better with locking.
| Err(e) => { | ||
| // If starting notifications failed a second time, disconnect so | ||
| // that we start again from a clean state next time. | ||
| match sensor.connection_status { |
There was a problem hiding this comment.
Is the connection_status ever anything other than Connecting here?
There was a problem hiding this comment.
Hrm. I guess not. This suggests that we want to do the retrying here until SENSOR_CONECT_LOCK_TIMEOUT/2 or something and kill off the failed once state? I think this is something I wanted to do anyway eventually.
There was a problem hiding this comment.
Fixed by moving the clone_sensor_at_idx call in connect_sensor_at_idx higher up, so it clones the sensor before changing the state of the original to Connecting.
| .bt_session | ||
| .connect(&sensor.id) | ||
| .await | ||
| .with_context(|| std::line!().to_string())?; |
| Err(e) => { | ||
| // If starting notifications failed a second time, disconnect so | ||
| // that we start again from a clean state next time. | ||
| match sensor.connection_status { |
There was a problem hiding this comment.
Hrm. I guess not. This suggests that we want to do the retrying here until SENSOR_CONECT_LOCK_TIMEOUT/2 or something and kill off the failed once state? I think this is something I wanted to do anyway eventually.
| sensors_connected: &mut Vec<Sensor>, | ||
| sensors_to_connect: &mut VecDeque<Sensor>, | ||
| /// If the sensor hasn't sent any updates in a while, disconnect it so we will try to reconnect. | ||
| async fn check_for_stale_sensor( |
There was a problem hiding this comment.
If we fix the to-do then this could be called disconnect_if_stale()?
This means that the previous state is used by connect_start_sensor to determine the next state rather than always the 'Connecting' state. It also means that the state will revert to the previous state if connecting fails.
| } | ||
| let mut sensor = clone_sensor_at_idx(state.clone(), idx).await; | ||
| println!("Trying to connect to {}", sensor.name); | ||
| let status = connect_start_sensor(session, &mut sensor).await; |
There was a problem hiding this comment.
I'm a bit sad that connect_start_sensor() mutates sensor rather than just returning the new connection_status, but think let's merge this and I'll have a crack at it over the weekend.
Refactor SensorState to have a single set of sensors, and use states
Moving here instead of alsuren/reading-xiaomi-temp#32.
Fixes #1.