quickfix_get_destination #31

Merged
Athamantis merged 11 commits from quickfix_get_destination into main 2026-03-07 09:12:46 +00:00
Owner

It was not a quick fix.....
but the elevator runs! yay!

It was not a quick fix..... but the elevator runs! yay!
add calls handler functions
All checks were successful
/ build_and_test (push) Successful in 22s
/ Static Release Build (push) Has been skipped
0d2548acde
add call services in the elevator_controller
All checks were successful
/ build_and_test (push) Successful in 22s
/ Static Release Build (push) Has been skipped
ae964e3aa4
fix elevator calls
All checks were successful
/ build_and_test (push) Successful in 23s
/ Static Release Build (push) Has been skipped
2ca3fe6052
Athamantis changed title from quickfix_get_destination to WIP: quickfix_get_destination 2026-03-05 19:15:50 +00:00
Author
Owner

closes #15

closes #15
add some comments and remove unnessesary debug prints
All checks were successful
/ build_and_test (push) Successful in 22s
/ Static Release Build (push) Has been skipped
5f66558c04
add stop elevator when obstruction is active
All checks were successful
/ build_and_test (push) Successful in 23s
/ Static Release Build (push) Has been skipped
3aa4d98a2c
fix some comments
All checks were successful
/ build_and_test (push) Successful in 22s
/ Static Release Build (push) Has been skipped
d0e87de4b1
Athamantis changed title from WIP: quickfix_get_destination to quickfix_get_destination 2026-03-05 20:15:08 +00:00
Author
Owner

What needs to be looked at is the way of fetching uuids.
fetching based on what elevator is assigned to the call

What needs to be looked at is the way of fetching uuids. fetching based on what elevator is assigned to the call
sebgab requested changes 2026-03-06 17:31:38 +00:00
Dismissed
Owner

Is this intended to take ownership of Elevator, or should it use a reference?

Is this intended to take ownership of `Elevator`, or should it use a reference?
Author
Owner

?

?
Author
Owner

added &mut to it

added &mut to it
Athamantis marked this conversation as resolved
@ -133,1 +133,4 @@
{
system_state.write().unwrap().set_destination();
}
floor_destination = {
Owner

You are once again dropping a write lock, in favour of re-acquiring a read lock.

You are once again dropping a write lock, in favour of re-acquiring a read lock.
Author
Owner

fixed

fixed
Athamantis marked this conversation as resolved
@ -166,0 +172,4 @@
{
match system_state.write().unwrap().start_servicing_call() {
Ok(_) => {}
Err(_) => {
Owner

What does this return, and why do we just not care about the value?

What does this return, and why do we just not care about the value?
Author
Owner

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

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
Author
Owner

added a debug print

added a debug print
Athamantis marked this conversation as resolved
@ -166,0 +173,4 @@
match system_state.write().unwrap().start_servicing_call() {
Ok(_) => {}
Err(_) => {
panic!("Could not start servicing call");
Owner

Should we not be printing the error? Why are we discarding this information?

Should we not be printing the error? Why are we discarding this information?
Author
Owner

printing now the error

printing now the error
Owner

Why is this printed on a separate line, normally you go "(...) failed with error e".

Why is this printed on a separate line, normally you go "(...) failed with error e".
Author
Owner

updated

updated
Athamantis marked this conversation as resolved
@ -188,0 +206,4 @@
Ok(_) => {}
Err(_) => {
error!("Failed to end that call is serviced")
}
Owner

See previous comments about discarding information.

See previous comments about discarding information.
Author
Owner

added a print of the error

added a print of the error
Athamantis marked this conversation as resolved
@ -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)
Owner

Was this intended? Should we consider making the port an optional launch parameter?

Was this intended? Should we consider making the port an optional launch parameter?
Author
Owner

hmm maybe
and yes it was intented

hmm maybe and yes it was intented
sebgab marked this conversation as resolved
@ -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);
Owner

I think this should be debug

I think this should be debug
Athamantis marked this conversation as resolved
@ -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");
Owner

This should most definitely be info...

This should most definitely be info...
Athamantis marked this conversation as resolved
@ -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"),
Owner

I think this should be debug.

I think this should be debug.
Athamantis marked this conversation as resolved
@ -6,3 +7,4 @@
use std::{
cell::OnceCell,
collections::{HashMap, HashSet},
error,
Owner

Where do we use this? I assume this is an un-intended auto-import and was intended to be error from log?

Where do we use this? I assume this is an un-intended auto-import and was intended to be error from log?
Author
Owner

removed

removed
Athamantis marked this conversation as resolved
@ -55,2 +60,4 @@
// TODO: Check that the call is not a duplicate
info!(
"Added new hall call. Destination: {:?}, Direction {:?}",
Owner

Please remove the invisible unicode character between direction and the info

Please remove the invisible unicode character between direction and the info
Athamantis marked this conversation as resolved
@ -60,1 +69,4 @@
}
/// Get a reference to a given elevator
pub fn get_call_as_ref(&self, call_id: &SmallUid) -> Option<&ElevatorCall> {
Owner

Doccomment does not describe function

Doccomment does not describe function
Athamantis marked this conversation as resolved
@ -61,0 +80,4 @@
}
/// Get a mutable reference to a given elevator
fn get_call_as_mut_ref(&mut self, call_id: &SmallUid) -> Option<&mut ElevatorCall> {
Owner

Doccomment does not describe function

Doccomment does not describe function
Athamantis marked this conversation as resolved
@ -61,0 +90,4 @@
}
/// Gets a random SmallUid from the vector of calls
fn get_random_id(&mut self) -> Option<SmallUid> {
Owner

This function gets a random valid call ID, not just a random SmallUid.

This function gets a random _valid_ call ID, not just a random SmallUid.
Athamantis marked this conversation as resolved
@ -299,3 +314,1 @@
"#
);
error!("Update function is not implemented");
// error!("Update function is not implemented");
Owner

Yeah... I get why you commented this out....
Feel free to just delete it.

Yeah... I get why you commented this out.... Feel free to just delete it.
Athamantis marked this conversation as resolved
@ -302,0 +357,4 @@
let random_id = self.calls.get_random_id();
if random_id.is_none() {
self.elevators
Owner

Please remove these comments.

Please remove these comments.
Athamantis marked this conversation as resolved
@ -302,0 +360,4 @@
self.elevators
.get_own_elevator_as_mut_ref()
.set_destination(None);
}
Owner

What is the purpose of this if statement? Is the same not done in the following match statement?

What is the purpose of this if statement? Is the same not done in the following match statement?
Athamantis marked this conversation as resolved
@ -302,0 +369,4 @@
self.calls.get_call_as_ref(&rand_id).unwrap().destination,
));
self.calls
.get_call_as_mut_ref(&rand_id)
Owner

Does the set destination function print something? I feel like it should.

Does the set destination function print something? I feel like it should.
Author
Owner

added some print

added some print
Athamantis marked this conversation as resolved
@ -302,0 +394,4 @@
};
debug!("Starting sevicing call id {:?}", call_id);
self.calls
.get_call_as_mut_ref(&call_id)
Owner

I feel like this can be an info print honestly.

I feel like this can be an info print honestly.
Athamantis marked this conversation as resolved
@ -302,0 +385,4 @@
for calls in self.calls.calls.iter() {
info!(
"Call: Id {:?}, Destination: {:?}, Direction: {:?}",
calls.id, calls.destination, calls.direction
Owner

We use SmallUid, not UUID.

We use SmallUid, not UUID.
Athamantis marked this conversation as resolved
@ -302,0 +405,4 @@
for calls in self.calls.calls.iter() {
info!(
"Call vector: Id {:?}, Destination: {:?}, Direction: {:?}",
Owner

Should be called something like mark call as services. When I read the code I think this checks if a call is serviced.

Should be called something like mark call as services. When I read the code I think this checks if a call is serviced.
Athamantis marked this conversation as resolved
@ -302,0 +427,4 @@
}
None => {
error!("Failed to fetch uuid");
Err(())
Owner

This function name does not tell me anything about what uid we are fetching.

This function name does not tell me anything about what uid we are fetching.
Athamantis marked this conversation as resolved
@ -345,1 +353,4 @@
}
pub fn update_callstate(&mut self, state: CallState) {
self.state = state;
Owner

Should be named set callstate or something, update is a bit ambiguous.

Should be named set callstate or something, update is a bit ambiguous.
Author
Owner

fixed

fixed
Athamantis marked this conversation as resolved
fix review comments
All checks were successful
/ build_and_test (push) Successful in 22s
/ Static Release Build (push) Has been skipped
4a8e234d7d
one comment to trace instead of debug
All checks were successful
/ build_and_test (push) Successful in 22s
/ Static Release Build (push) Has been skipped
c42ef44a01
sebgab requested changes 2026-03-06 19:17:17 +00:00
Dismissed
@ -166,0 +170,4 @@
{
match system_state.write().unwrap().start_servicing_call() {
Ok(_) => {}
Owner

add print

add print
Athamantis marked this conversation as resolved
@ -187,0 +205,4 @@
Ok(_) => {}
Err(e) => {
error!("Failed to end that call is serviced");
error!("Error: {:?}", e);
Owner

See previous messages about log and discarding info.

See previous messages about log and discarding info.
Athamantis marked this conversation as resolved
@ -61,0 +71,4 @@
// Get our stored version of the same call
self.calls
.iter()
.find_map(|c| match c.id == call_id.to_owned() {
Owner

With your magic link I found out this is silly. If we use find instead of find_map we can ditch the map and only have the boolean comparison.

With your magic link I found out this is silly. If we use `find` instead of `find_map` we can ditch the map and only have the boolean comparison.
Author
Owner

fixed

fixed
Athamantis marked this conversation as resolved
@ -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() {
Owner

See previous commend about being a silly little goose

See previous commend about being a silly little goose
Author
Owner

fixed

fixed
Athamantis marked this conversation as resolved
@ -302,0 +341,4 @@
.get_own_elevator_as_mut_ref()
.set_destination(None);
trace!("Destination of the elevator is set to None",);
Owner

If the other one is info, I feel it makes sense to make this info too maybe?

If the other one is info, I feel it makes sense to make this info too maybe?
Author
Owner

ehhh no. if you want a filled terminal, be my guest, but i prerer acctually reading the other log messages

ehhh no. if you want a filled terminal, be my guest, but i prerer acctually reading the other log messages
Athamantis marked this conversation as resolved
@ -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> {
Owner

Calls do not have uuids, just use the generic "id"

Calls do not have uuids, just use the generic "id"
Athamantis marked this conversation as resolved
add ok print comments and combinde error message
All checks were successful
/ build_and_test (push) Successful in 22s
/ Static Release Build (push) Has been skipped
97ab7417d0
moved from find map to just map
All checks were successful
/ build_and_test (push) Successful in 23s
/ Static Release Build (push) Has been skipped
db98a3dde5
resolve review comments
All checks were successful
/ build_and_test (push) Successful in 22s
/ Static Release Build (push) Has been skipped
da0a0f5d34
sebgab approved these changes 2026-03-07 09:12:29 +00:00
sebgab left a comment
Owner

LGTM

LGTM
Athamantis deleted branch quickfix_get_destination 2026-03-07 09:12:47 +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!31
No description provided.