elevator_controller #20

Merged
Athamantis merged 25 commits from elevator_controller into main 2026-03-04 17:58:14 +00:00
Owner
No description provided.
add psudo code for the elevator_controller
All checks were successful
/ build_and_test (push) Successful in 23s
5392eddf0e
add includes for the elevator_controller
Some checks failed
/ build_and_test (push) Failing after 16s
84d26d319f
add system state to the elevator controller function
Some checks failed
/ build_and_test (push) Failing after 22s
eef35be564
Add logger and comments
Some checks failed
/ build_and_test (push) Failing after 24s
658db9f49d
add comment
Some checks failed
/ build_and_test (push) Failing after 20s
b77aa3ddd6
made ElevatorInterface pub
All checks were successful
/ build_and_test (push) Successful in 33s
61ae49bace
add fetch state, and set the elevator to stop
All checks were successful
/ build_and_test (push) Successful in 31s
100b324970
fix scooping for the read write lock
All checks were successful
/ build_and_test (push) Successful in 32s
f600e2067a
remove unused code
All checks were successful
/ build_and_test (push) Successful in 30s
e70fd9922c
Merge branch 'main' into elevator_controller
All checks were successful
/ build_and_test (push) Successful in 33s
ce4109bb45
checking out door handler instead
Some checks failed
/ build_and_test (push) Failing after 21s
a64ca67ce8
Merge branch 'main' into elevator_controller
Some checks failed
/ build_and_test (push) Failing after 14s
/ Static Release Build (push) Has been skipped
1e589da4cc
fix the elevator_controller logic
Some checks failed
/ build_and_test (push) Failing after 14s
/ Static Release Build (push) Has been skipped
6a42173dd5
add floor handler task
Some checks failed
/ build_and_test (push) Failing after 15s
/ Static Release Build (push) Has been skipped
ec11317091
add tests that does not work
Some checks failed
/ build_and_test (push) Failing after 21s
/ Static Release Build (push) Has been skipped
b9c924eba5
add await and async to the floor_handler test
All checks were successful
/ build_and_test (push) Successful in 21s
/ Static Release Build (push) Has been skipped
e828eae851
change current floor to an option, and finish floor_handler tests
All checks were successful
/ build_and_test (push) Successful in 20s
/ Static Release Build (push) Has been skipped
de5cca5b0a
add floor handler to spawn
All checks were successful
/ build_and_test (push) Successful in 20s
/ Static Release Build (push) Has been skipped
9a10f840a3
add elevator_controller call in main
All checks were successful
/ build_and_test (push) Successful in 21s
/ Static Release Build (push) Has been skipped
87fffc9660
update comments in program
All checks were successful
/ build_and_test (push) Successful in 21s
/ Static Release Build (push) Has been skipped
00aa44eb2f
Athamantis changed title from WIP: elevator_controller to elevator_controller 2026-03-03 11:39:02 +00:00
Owner

Why have we added the SimElevatorServer binary?

Why have we added the `SimElevatorServer` binary?
sebgab requested changes 2026-03-03 12:14:25 +00:00
Dismissed
@ -0,0 +1,494 @@
//! # TTK4145 Elevator Driver
Owner

What is this file?

What is this file?
Author
Owner

its the ttk4145 elevator driver documentation

its the ttk4145 elevator driver documentation
Owner

Why is this included?

Why is this included?
Author
Owner

because i got annoyed of reading it on matrix

because i got annoyed of reading it on matrix
Author
Owner

removed

removed
Athamantis marked this conversation as resolved
@ -0,0 +150,4 @@
/// Elevator controller
///
/// Takes in SystemState and does not return anything.
Owner

The function signature tells me this, why is it in the comment?

The function signature tells me this, why is it in the comment?
Author
Owner

removed

removed
Athamantis marked this conversation as resolved
@ -0,0 +158,4 @@
///
/// After arriving at the desired destination the doors will open and stay open for three seconds.
///
/// @breif elevator_controller makes sure the elevator is at right destination before opening
Owner

Remove @brief and @param, those are not Rust things...

Remove `@brief` and `@param`, those are not Rust things...
Author
Owner

deleted

deleted
Athamantis marked this conversation as resolved
@ -0,0 +24,4 @@
.get_own_elevator_as_ref()
.get_current_floor()
};
while current_floor_option == None {
Owner

We should yield in this loop.

We should yield in this loop.
Author
Owner

added

added
Athamantis marked this conversation as resolved
@ -0,0 +167,4 @@
let floor_destination: Destination = get_floor_destination(system_state.clone()).await;
info!(
"Starting elevator controller to move to floor {:?}",
Owner

Non-blocker: Consider implementing Display for Destination so we don't have to debug print.

Non-blocker: Consider implementing `Display` for `Destination` so we don't have to debug print.
Author
Owner

?
not familiar with

? not familiar with
Owner

The Display trait is what is used to you can non-debug print something.

The [Display trait](https://doc.rust-lang.org/std/fmt/trait.Display.html) is what is used to you can non-debug print something.
Author
Owner

this???

/ The destination of an [ElevatorCall]
#[derive(Clone, Copy, Debug, Eq, PartialEq, Hash, Serialize, Deserialize, PartialOrd, Ord)]
pub enum Destination {
    Floor1,
    Floor2,
    Floor3,
    Floor4,
}
impl fmt::Display for Destination {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "{}", self)
    }
}

this??? ``` / The destination of an [ElevatorCall] #[derive(Clone, Copy, Debug, Eq, PartialEq, Hash, Serialize, Deserialize, PartialOrd, Ord)] pub enum Destination { Floor1, Floor2, Floor3, Floor4, } impl fmt::Display for Destination { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "{}", self) } } ```
Owner

Yep, just gotta write the function body now.

Yep, just gotta write the function body now.
Athamantis marked this conversation as resolved
@ -0,0 +40,4 @@
/// waiting_for_door_to_be_closed checks the current door state, and while the door is not closed
/// it will stop the elevator, update the system state, and then try to close the doors again
async fn waiting_for_door_to_be_closed(system_state: Arc<RwLock<SystemState>>, elevator: Elevator) {
Owner

Pedantic, but should be wait_for(...) not waiting_for(...)

Pedantic, but should be `wait_for(...)` not `waiting_for(...)`
Author
Owner

oki

oki
Athamantis marked this conversation as resolved
@ -0,0 +46,4 @@
state.elevators.get_own_elevator_as_ref().get_door_state()
};
while current_door_state != DoorState::Closed {
Owner

Loop should yield.

Loop should yield.
Author
Owner

added

added
Athamantis marked this conversation as resolved
@ -0,0 +65,4 @@
}
current_door_state = {
let state = system_state.read().unwrap();
Owner

Why do we acquire yet another lock? Why do we not just use the previous lock?

Why do we acquire yet another lock? Why do we not just use the previous lock?
Author
Owner

wdym?

wdym?
Owner

At (1) we acquire the Mutex write lock, waiting for potentially quite a while to get it.
At (2) we drop the Mutex write lock.
At (3) we acquire a Mutex read lock, potentially waiting a bit to get it.

Why do we have to do (2) and (3) when we could just use the lock acquired at (1)?

At (1) we acquire the Mutex write lock, waiting for potentially quite a while to get it. At (2) we drop the Mutex write lock. At (3) we acquire a Mutex read lock, potentially waiting a bit to get it. Why do we have to do (2) and (3) when we could just use the lock acquired at (1)?
Author
Owner

fixed

fixed
Athamantis marked this conversation as resolved
@ -0,0 +60,4 @@
{
let mut state_writer = system_state.write().unwrap();
state_writer.elevators.close_door();
Owner

Why is this in another closure? Could we not just use the write lock in the previous closure?

Why is this in another closure? Could we not just use the write lock in the previous closure?
Author
Owner

i like {}
so my goal was to have as many as posible without getting comments on it

i like {} so my goal was to have as many as posible without getting comments on it
Athamantis marked this conversation as resolved
@ -0,0 +83,4 @@
state.elevators.get_own_elevator_as_ref().get_obstructed()
};
while obstructed_state != None {
Owner

Btw in general we should use the is_some() function rather than != None.
You don't need to fix it for this PR, but it is something you would have known if you'd run cargo clippy

Btw in general we should use the `is_some()` function rather than `!= None`. You don't need to fix it for this PR, but it is something you would have known if you'd run `cargo clippy`
Author
Owner

ooo did not know of
done!

ooo did not know of done!
Athamantis marked this conversation as resolved
@ -0,0 +95,4 @@
/// Function takes in two destinations and returns an [`Direction`] based on which way the elevator
/// needs to drive to get from [`current_floor`] to [`floor_destination`].
fn calculate_direction(floor_destination: Destination, current_floor: Destination) -> Direction {
Owner

Why is this a separate function, could this not be made into:

impl Destination {
  fn direction_to(&self, target: Self) -> Option<Direction> {}
}

Then to use it you can just go .direction_to on a direction.

Why is this a separate function, could this not be made into: ```Rust impl Destination { fn direction_to(&self, target: Self) -> Option<Direction> {} } ``` Then to use it you can just go `.direction_to` on a direction.
Author
Owner

good point
i just love to make functions only i can use

good point i just love to make functions only i can use
Author
Owner

added

added
Athamantis marked this conversation as resolved
@ -0,0 +96,4 @@
/// Function takes in two destinations and returns an [`Direction`] based on which way the elevator
/// needs to drive to get from [`current_floor`] to [`floor_destination`].
fn calculate_direction(floor_destination: Destination, current_floor: Destination) -> Direction {
match current_floor > floor_destination {
Owner

This code insinuates that if the current floor and destination floor is the same we need to move.

This code insinuates that if the current floor and destination floor is the same we need to move.
Author
Owner

added option, if the same then return None

added option, if the same then return None
Athamantis marked this conversation as resolved
@ -0,0 +116,4 @@
// Check if door is closed, and wait for it to be
waiting_for_door_to_be_closed(system_state.clone(), elevator.clone()).await;
// no -> move elevator
Owner

I have no idea what this comment means.

I have no idea what this comment means.
Author
Owner

removed

removed
Athamantis marked this conversation as resolved
@ -0,0 +120,4 @@
elevator.motor_direction(direction.into());
{
// Updates the system state
let mut state = system_state.write().unwrap();
Owner

If it takes a while to get the write lock we will already have moved a bit before the state is updated.
Consider waiting to update the state, before moving.

If it takes a while to get the write lock we will already have moved a bit before the state is updated. Consider waiting to update the state, _before_ moving.
Author
Owner

switched

switched
Athamantis marked this conversation as resolved
@ -0,0 +138,4 @@
state.elevators.get_own_elevator_as_ref().get_destination()
};
while floor_destination == None {
Owner

Loop should yield

Loop should yield
Author
Owner

added yay!

added yay!
Athamantis marked this conversation as resolved
@ -0,0 +223,4 @@
use crate::system_state_controller::SystemState;
#[tokio::test]
async fn test_test() {
Owner

This is an empty test, please remove.

This is an empty test, please remove.
Author
Owner

added one test

added one test
Athamantis marked this conversation as resolved
@ -0,0 +174,4 @@
let mut current_floor =
get_current_floor(system_state.clone(), elevator.clone(), Direction::Down).await;
while current_floor != floor_destination {
Owner

Loop should yield

Loop should yield
Author
Owner

added

added
Athamantis marked this conversation as resolved
@ -0,0 +172,4 @@
);
let mut current_floor =
get_current_floor(system_state.clone(), elevator.clone(), Direction::Down).await;
Owner

Why is down hardcoded here?

Why is down hardcoded here?
Author
Owner

because i need to have one direction when getting the current floor, because if there is no floor then the elevator needs to move, and here i am just picking a direction

because i need to have one direction when getting the current floor, because if there is no floor then the elevator needs to move, and here i am just picking a direction
sebgab marked this conversation as resolved
@ -0,0 +186,4 @@
current_floor = get_current_floor(system_state.clone(), elevator.clone(), direction).await;
}
elevator.motor_direction(DIRN_STOP);
Owner

See previous comment about updating state first.

See previous comment about updating state first.
Author
Owner

eh, here i think we want to stop the elevator at once, not wait until we have the lock so stop, as you said it can take a little bitt aquiring the lock, and therefore we should stop at once the elevator is at the right posision before we move it further away from the destination

eh, here i think we want to stop the elevator at once, not wait until we have the lock so stop, as you said it can take a little bitt aquiring the lock, and therefore we should stop at once the elevator is at the right posision before we move it further away from the destination
Owner

Good point, we should not wait for a state lock before stopping.

Good point, we should not wait for a state lock before stopping.
sebgab marked this conversation as resolved
src/main.rs Outdated
@ -66,6 +67,7 @@ async fn main() {
if elevator.floor_sensor().is_none() {
elevator.motor_direction(dirn);
}
// elevator.motor_direction(elev::DIRN_UP);
Owner

Remove commented out code.

Remove commented out code.
Author
Owner

done

done
Athamantis marked this conversation as resolved
@ -0,0 +4,4 @@
use driver_rust::elevio::poll::obstruction;
use log::{info, trace};
use std::sync::{Arc, RwLock};
// use crossbeam_channel::{self as cbc, Receiver};
Owner

Delete commented out import

Delete commented out import
Author
Owner

done

done
Athamantis marked this conversation as resolved
@ -0,0 +8,4 @@
use log::debug;
use tokio::sync::mpsc;
async fn floor_handler(state: Arc<RwLock<SystemState>>, floor_rx: &Receiver<u8>) {
Owner

Missing doccomment & function does not need to be async.

Missing doccomment & function does not need to be async.
Author
Owner

doesnt it?
it has an .recv() which means we need to wait for info geting updated

doesnt it? it has an .recv() which means we need to wait for info geting updated
Owner

You only need async if you have a .await* entry. I haven't double checked, but I believe CBC, like the std implementation is blocking on wait.

  • There are maybe some others too, but don't think about those
You only need async if you have a `.await`* entry. I haven't double checked, but I believe `CBC`, like the `std` implementation is blocking on wait. * There are maybe _some_ others too, but don't think about those
Author
Owner

okay, fixed

okay, fixed
Athamantis marked this conversation as resolved
@ -0,0 +13,4 @@
let recieved_floor: u8 = floor_rx.recv().unwrap();
trace!("Recieved from floor controller {:?}", recieved_floor);
// TODO: Find a more modual way to do this
Owner

Consider: impl TryFrom<u8> for Destination

Consider: `impl TryFrom<u8> for Destination`
Author
Owner

added!

added!
Athamantis marked this conversation as resolved
@ -3,3 +4,3 @@
use serde::{Deserialize, Serialize};
use small_uid::SmallUid;
use std::{collections::HashSet, net::IpAddr};
use std::{collections::HashSet, net::IpAddr, u8};
Owner

Do we really need to import u8??

Do we really need to import `u8`??
Author
Owner

ehhh i think it just added itself

ehhh i think it just added itself
Athamantis marked this conversation as resolved
@ -173,0 +190,4 @@
/// Set the current location of the elevator
pub fn set_current_floor(&mut self, floor: Option<Destination>) {
if self.current_floor != floor {
Owner

The exact same happens regardless of if this if statement is here, consider removing to keep things tidy.

The exact same happens regardless of if this `if` statement is here, consider removing to keep things tidy.
Author
Owner

?
i thougth the tx would send everytime something updates, so if the floors are alike then we probably does not need to do anything

? i thougth the tx would send everytime something updates, so if the floors are alike then we probably does not need to do anything
Owner

The TX handler sends everytime a get_as_mut_ref function is called.

The TX handler sends everytime a `get_as_mut_ref` function is called.
Author
Owner

ah okay, deleted

ah okay, deleted
Athamantis marked this conversation as resolved
Fix some review comments
All checks were successful
/ build_and_test (push) Successful in 21s
/ Static Release Build (push) Has been skipped
0fb7e4a131
fix some commets
All checks were successful
/ build_and_test (push) Successful in 21s
/ Static Release Build (push) Has been skipped
c2063aefc8
fix some clippy warnings
All checks were successful
/ build_and_test (push) Successful in 21s
/ Static Release Build (push) Has been skipped
05b769d6f9
removed unnessesary files
All checks were successful
/ build_and_test (push) Successful in 21s
/ Static Release Build (push) Has been skipped
ba4a3f15ec
sebgab approved these changes 2026-03-04 17:36:20 +00:00
sebgab left a comment
Owner

Assuming all is good because you definitely fixed all my comments and I am too tired to do a proper review of this code and it would be nice to get it merged today so we can test rather than waiting for Friday.

:02love:

Assuming all is good because you definitely fixed all my comments and I am too tired to do a proper review of this code and it would be nice to get it merged today so we can test rather than waiting for Friday. :02love:
Athamantis deleted branch elevator_controller 2026-03-04 17:58:24 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
TTK4145/elevator!20
No description provided.