elevator_controller #20
No reviewers
Labels
No labels
bug
duplicate
enhancement
help wanted
invalid
priority
high
priority
low
priority
medium
question
wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
TTK4145/elevator!20
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "elevator_controller"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
WIP: elevator_controllerto elevator_controllerWhy have we added the
SimElevatorServerbinary?@ -0,0 +1,494 @@//! # TTK4145 Elevator DriverWhat is this file?
its the ttk4145 elevator driver documentation
Why is this included?
because i got annoyed of reading it on matrix
removed
@ -0,0 +150,4 @@/// Elevator controller////// Takes in SystemState and does not return anything.The function signature tells me this, why is it in the comment?
removed
@ -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 openingRemove
@briefand@param, those are not Rust things...deleted
@ -0,0 +24,4 @@.get_own_elevator_as_ref().get_current_floor()};while current_floor_option == None {We should yield in this loop.
added
@ -0,0 +167,4 @@let floor_destination: Destination = get_floor_destination(system_state.clone()).await;info!("Starting elevator controller to move to floor {:?}",Non-blocker: Consider implementing
DisplayforDestinationso we don't have to debug print.?
not familiar with
The Display trait is what is used to you can non-debug print something.
this???
Yep, just gotta write the function body now.
@ -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 againasync fn waiting_for_door_to_be_closed(system_state: Arc<RwLock<SystemState>>, elevator: Elevator) {Pedantic, but should be
wait_for(...)notwaiting_for(...)oki
@ -0,0 +46,4 @@state.elevators.get_own_elevator_as_ref().get_door_state()};while current_door_state != DoorState::Closed {Loop should yield.
added
@ -0,0 +65,4 @@}current_door_state = {let state = system_state.read().unwrap();Why do we acquire yet another lock? Why do we not just use the previous lock?
wdym?
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)?
fixed
@ -0,0 +60,4 @@{let mut state_writer = system_state.write().unwrap();state_writer.elevators.close_door();Why is this in another closure? Could we not just use the write lock in the previous closure?
i like {}
so my goal was to have as many as posible without getting comments on it
@ -0,0 +83,4 @@state.elevators.get_own_elevator_as_ref().get_obstructed()};while obstructed_state != None {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 clippyooo did not know of
done!
@ -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 {Why is this a separate function, could this not be made into:
Then to use it you can just go
.direction_toon a direction.good point
i just love to make functions only i can use
added
@ -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 {This code insinuates that if the current floor and destination floor is the same we need to move.
added option, if the same then return None
@ -0,0 +116,4 @@// Check if door is closed, and wait for it to bewaiting_for_door_to_be_closed(system_state.clone(), elevator.clone()).await;// no -> move elevatorI have no idea what this comment means.
removed
@ -0,0 +120,4 @@elevator.motor_direction(direction.into());{// Updates the system statelet mut state = system_state.write().unwrap();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.
switched
@ -0,0 +138,4 @@state.elevators.get_own_elevator_as_ref().get_destination()};while floor_destination == None {Loop should yield
added yay!
@ -0,0 +223,4 @@use crate::system_state_controller::SystemState;#[tokio::test]async fn test_test() {This is an empty test, please remove.
added one test
@ -0,0 +174,4 @@let mut current_floor =get_current_floor(system_state.clone(), elevator.clone(), Direction::Down).await;while current_floor != floor_destination {Loop should yield
added
@ -0,0 +172,4 @@);let mut current_floor =get_current_floor(system_state.clone(), elevator.clone(), Direction::Down).await;Why is down hardcoded here?
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
@ -0,0 +186,4 @@current_floor = get_current_floor(system_state.clone(), elevator.clone(), direction).await;}elevator.motor_direction(DIRN_STOP);See previous comment about updating state first.
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
Good point, we should not wait for a state lock before stopping.
@ -66,6 +67,7 @@ async fn main() {if elevator.floor_sensor().is_none() {elevator.motor_direction(dirn);}// elevator.motor_direction(elev::DIRN_UP);Remove commented out code.
done
@ -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};Delete commented out import
done
@ -0,0 +8,4 @@use log::debug;use tokio::sync::mpsc;async fn floor_handler(state: Arc<RwLock<SystemState>>, floor_rx: &Receiver<u8>) {Missing doccomment & function does not need to be async.
doesnt it?
it has an .recv() which means we need to wait for info geting updated
You only need async if you have a
.await* entry. I haven't double checked, but I believeCBC, like thestdimplementation is blocking on wait.okay, fixed
@ -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 thisConsider:
impl TryFrom<u8> for Destinationadded!
@ -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};Do we really need to import
u8??ehhh i think it just added itself
@ -173,0 +190,4 @@/// Set the current location of the elevatorpub fn set_current_floor(&mut self, floor: Option<Destination>) {if self.current_floor != floor {The exact same happens regardless of if this
ifstatement is here, consider removing to keep things tidy.?
i thougth the tx would send everytime something updates, so if the floors are alike then we probably does not need to do anything
The TX handler sends everytime a
get_as_mut_reffunction is called.ah okay, deleted
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: