quickfix_get_destination #31
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!31
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "quickfix_get_destination"
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?
It was not a quick fix.....
but the elevator runs! yay!
quickfix_get_destinationto WIP: quickfix_get_destinationcloses #15
WIP: quickfix_get_destinationto quickfix_get_destinationWhat needs to be looked at is the way of fetching uuids.
fetching based on what elevator is assigned to the call
Is this intended to take ownership of
Elevator, or should it use a reference??
added &mut to it
@ -133,1 +133,4 @@{system_state.write().unwrap().set_destination();}floor_destination = {You are once again dropping a write lock, in favour of re-acquiring a read lock.
fixed
@ -166,0 +172,4 @@{match system_state.write().unwrap().start_servicing_call() {Ok(_) => {}Err(_) => {What does this return, and why do we just not care about the value?
it returns if it found the call in the service or not, and it successfully updated the call to that we are handling it.
we only care of its failiure, not of its succsess
added a debug print
@ -166,0 +173,4 @@match system_state.write().unwrap().start_servicing_call() {Ok(_) => {}Err(_) => {panic!("Could not start servicing call");Should we not be printing the error? Why are we discarding this information?
printing now the error
Why is this printed on a separate line, normally you go "(...) failed with error e".
updated
@ -188,0 +206,4 @@Ok(_) => {}Err(_) => {error!("Failed to end that call is serviced")}See previous comments about discarding information.
added a print of the error
@ -34,3 +34,3 @@let elev_num_floors = 4;let elevator = elev::Elevator::init("localhost:15657", elev_num_floors)let elevator = elev::Elevator::init("localhost:3113", elev_num_floors)Was this intended? Should we consider making the port an optional launch parameter?
hmm maybe
and yes it was intented
@ -30,3 +31,3 @@let sender: IpAddr = match socket.recv_from(&mut recv_buf).await {Ok((amount, sender)) => {debug!("Received {} from {}", amount, sender);trace!("Received {} from {}", amount, sender);I think this should be debug
@ -58,3 +59,3 @@/// Initializes the RX handler and runs the [rx_handler] in an infinite loop.pub async fn rx_handler_task(state: Arc<RwLock<SystemState>>) -> ! {info!("Starting RX handler");trace!("Starting RX handler");This should most definitely be info...
@ -80,2 +79,3 @@trace!("Sending {} bytes of data", data.len());match socket.send_to(&data, "255.255.255.255:21434").await {Ok(amount) => debug!("Transmitted {amount} sucessfully"),Ok(amount) => trace!("Transmitted {amount} sucessfully"),I think this should be debug.
@ -6,3 +7,4 @@use std::{cell::OnceCell,collections::{HashMap, HashSet},error,Where do we use this? I assume this is an un-intended auto-import and was intended to be error from log?
removed
@ -55,2 +60,4 @@// TODO: Check that the call is not a duplicateinfo!("Added new hall call. Destination: {:?}, Direction {:?}",Please remove the invisible unicode character between direction and the info
@ -60,1 +69,4 @@}/// Get a reference to a given elevatorpub fn get_call_as_ref(&self, call_id: &SmallUid) -> Option<&ElevatorCall> {Doccomment does not describe function
@ -61,0 +80,4 @@}/// Get a mutable reference to a given elevatorfn get_call_as_mut_ref(&mut self, call_id: &SmallUid) -> Option<&mut ElevatorCall> {Doccomment does not describe function
@ -61,0 +90,4 @@}/// Gets a random SmallUid from the vector of callsfn get_random_id(&mut self) -> Option<SmallUid> {This function gets a random valid call ID, not just a random SmallUid.
@ -299,3 +314,1 @@"#);error!("Update function is not implemented");// error!("Update function is not implemented");Yeah... I get why you commented this out....
Feel free to just delete it.
@ -302,0 +357,4 @@let random_id = self.calls.get_random_id();if random_id.is_none() {self.elevatorsPlease remove these comments.
@ -302,0 +360,4 @@self.elevators.get_own_elevator_as_mut_ref().set_destination(None);}What is the purpose of this if statement? Is the same not done in the following match statement?
@ -302,0 +369,4 @@self.calls.get_call_as_ref(&rand_id).unwrap().destination,));self.calls.get_call_as_mut_ref(&rand_id)Does the set destination function print something? I feel like it should.
added some print
@ -302,0 +394,4 @@};debug!("Starting sevicing call id {:?}", call_id);self.calls.get_call_as_mut_ref(&call_id)I feel like this can be an info print honestly.
@ -302,0 +385,4 @@for calls in self.calls.calls.iter() {info!("Call: Id {:?}, Destination: {:?}, Direction: {:?}",calls.id, calls.destination, calls.directionWe use SmallUid, not UUID.
@ -302,0 +405,4 @@for calls in self.calls.calls.iter() {info!("Call vector: Id {:?}, Destination: {:?}, Direction: {:?}",Should be called something like mark call as services. When I read the code I think this checks if a call is serviced.
@ -302,0 +427,4 @@}None => {error!("Failed to fetch uuid");Err(())This function name does not tell me anything about what uid we are fetching.
@ -345,1 +353,4 @@}pub fn update_callstate(&mut self, state: CallState) {self.state = state;Should be named set callstate or something, update is a bit ambiguous.
fixed
@ -166,0 +170,4 @@{match system_state.write().unwrap().start_servicing_call() {Ok(_) => {}add print
@ -187,0 +205,4 @@Ok(_) => {}Err(e) => {error!("Failed to end that call is serviced");error!("Error: {:?}", e);See previous messages about log and discarding info.
@ -61,0 +71,4 @@// Get our stored version of the same callself.calls.iter().find_map(|c| match c.id == call_id.to_owned() {With your magic link I found out this is silly. If we use
findinstead offind_mapwe can ditch the map and only have the boolean comparison.fixed
@ -61,0 +81,4 @@fn get_call_as_mut_ref(&mut self, call_id: &SmallUid) -> Option<&mut ElevatorCall> {self.calls.iter_mut().find_map(|c| match c.id == call_id.to_owned() {See previous commend about being a silly little goose
fixed
@ -302,0 +341,4 @@.get_own_elevator_as_mut_ref().set_destination(None);trace!("Destination of the elevator is set to None",);If the other one is info, I feel it makes sense to make this info too maybe?
ehhh no. if you want a filled terminal, be my guest, but i prerer acctually reading the other log messages
@ -302,0 +393,4 @@/// Fetches the Small UUID from the list of calls, based on if the [`handling_elevator`] is the same/// as the elevator id.fn fetch_call_uuid(&mut self) -> Option<SmallUid> {Calls do not have uuids, just use the generic "id"
LGTM