Add: Device module and example for Ping1D #23

RaulTrombin · 2024-04-18T17:17:35Z

No description provided.

joaoantoniocardoso · 2024-04-24T18:06:28Z

src/device.rs

+    async fn stream(
+        mut serial_stream: SplitStream<Framed<SerialStream, PingCodec>>,
+        broadcast_tx: Sender<ProtocolMessage>,
+    ) {
+        'serial_stream: loop {
+            while let Some(msg) = serial_stream.next().await {
+                match msg {
+                    Ok(msg) => {
+                        if let Err(_e) = broadcast_tx.send(msg) {
+                            dbg! {_e};
+                            break 'serial_stream;
+                        };
+                    }
+                    Err(e) => {
+                        dbg!(e);
+                    }
+                }
+            }
+        }
+    }


This task is likely to leave forever, as the only way out of the loop is if the broadcast_tx fails to send (by being closed), but who closes it?

Is there a master "panic!" for this use case?
I believe if it get some faltal error on serial bus it should stop.
We can create a manager which retry stabilish the port, but I not but the port creation inside the device yet.

patrickelectric · 2024-04-25T11:56:57Z

examples/ping_1d.rs

+    Ok(())
+}
+
+mod cli_args {


use structs over clap builder pattern, it's simpler and easier to read/maintain.

k, please check the newer version

patrickelectric · 2024-04-25T11:57:48Z

examples/ping_1d.rs

+
+    let mut port = tokio_serial::new(port_name, baud_rate).open_native_async()?;
+    #[cfg(unix)]
+    port.set_exclusive(false)?;


What is the reason for this ? Is missing a comment.

I believe it's to allow reuse the ports in the same scope,
https://docs.rs/tokio-serial/latest/tokio_serial/struct.SerialStream.html#method.set_exclusive

It was into example tokio_serial, still works without it, let remove and keep it to future if it's required

patrickelectric · 2024-04-25T11:58:37Z

examples/ping_1d.rs

+                }
+                Err(_e) => break,
+            }
+            if ProfileStructVector.len() == 30 {


patrickelectric · 2024-04-25T12:00:59Z

src/device.rs

+                        if !(common::AckStruct::id() == answer.message_id
+                            || common::NackStruct::id() == answer.message_id)


common::AckStruct::id() != answer.message_id && common::NackStruct::id() != answer.message_id

patrickelectric · 2024-04-25T12:02:14Z

src/device.rs

+                                if answer.nacked_id != message_id {
+                                    continue;
+                                };
+                                return Err(PingError::NackError(answer));


The answer here is unnecessary, there is not useful inside

For logging it could be usefull, since it have the request id inside it.
Should we log and return error? I think a generic error will be harder to track

Since Nack contains the error contains nack_message https://docs.bluerobotics.com/ping-protocol/pingmessage-common/#2-nack it's better to return the message itself or the struct.
Err(PingError::NackError(String))

patrickelectric · 2024-04-25T12:02:54Z

src/device.rs

+                                };
+                                return Err(PingError::NackError(answer));
+                            }
+                            _ => return Err(PingError::TryFromError(answer)), // Almost unreachable, but raises error ProtocolMessage


Shoudnt this be a continue ?

I think we never reach this condition. If it's an Ack or Nack, it verifies the ID.
This is just in case any error happens during conversion (which will be almost impossible, as we do CRC during the message reception.)

patrickelectric · 2024-04-25T12:04:27Z

src/device.rs

+        };
+
+        match tokio::time::timeout(tokio::time::Duration::from_secs(15), future).await {
+            Ok(_result) => Ok(()),


This should return the future result, not an empty ok

patrickelectric · 2024-04-25T12:09:18Z

src/device.rs

+                match msg {
+                    Ok(msg) => {
+                        if let Err(_e) = broadcast_tx.send(msg) {
+                            dbg! {_e};


This should use parentheses not braces

changed to tracing, since we merged last related PR

src/device.rs

examples/ping_1d.rs

patrickelectric · 2024-04-25T18:21:18Z

src/device.rs

+                                if answer.acked_id != message_id {
+                                    continue;
+                                };
+                                return Ok(answer);


In this case, we should return an empty Ok since ack will always contain message_id
https://docs.bluerobotics.com/ping-protocol/pingmessage-common/#1-ack

patrickelectric · 2024-04-25T18:22:25Z

src/device.rs

+                                if answer.nacked_id != message_id {
+                                    continue;
+                                };
+                                return Err(PingError::NackError(answer));


Since Nack contains the error contains nack_message https://docs.bluerobotics.com/ping-protocol/pingmessage-common/#2-nack it's better to return the message itself or the struct.
Err(PingError::NackError(String))

…hods structure

RaulTrombin · 2024-04-25T19:03:16Z

@patrickelectric with you

RaulTrombin marked this pull request as draft April 18, 2024 17:17

RaulTrombin requested review from patrickelectric and joaoantoniocardoso April 18, 2024 17:17

RaulTrombin force-pushed the creave_device_inherit branch 4 times, most recently from 67d02f2 to db893ec Compare April 22, 2024 22:56

RaulTrombin marked this pull request as ready for review April 22, 2024 22:58

RaulTrombin force-pushed the creave_device_inherit branch 5 times, most recently from b4cb7aa to c614a09 Compare April 24, 2024 16:08

RaulTrombin marked this pull request as draft April 24, 2024 16:11

RaulTrombin changed the title ~~Add device module and example [WIP]~~ Add: Device module and example for Ping1D Apr 24, 2024

RaulTrombin marked this pull request as ready for review April 24, 2024 16:11

RaulTrombin mentioned this pull request Apr 24, 2024

Add first methods and structure for PingDevice #9

Closed

joaoantoniocardoso reviewed Apr 24, 2024

View reviewed changes

patrickelectric requested changes Apr 25, 2024

View reviewed changes

RaulTrombin force-pushed the creave_device_inherit branch from c614a09 to fbb3147 Compare April 25, 2024 14:58

RaulTrombin requested a review from patrickelectric April 25, 2024 14:59

RaulTrombin force-pushed the creave_device_inherit branch 2 times, most recently from b70f6df to 07d42bf Compare April 25, 2024 15:01

RaulTrombin requested a review from joaoantoniocardoso April 25, 2024 16:10

patrickelectric requested changes Apr 25, 2024

View reviewed changes

RaulTrombin force-pushed the creave_device_inherit branch from 07d42bf to 3c7b360 Compare April 25, 2024 18:28

patrickelectric force-pushed the creave_device_inherit branch 2 times, most recently from d2af646 to 9f9861b Compare April 25, 2024 18:50

RaulTrombin added 2 commits April 25, 2024 16:00

Cargo: Move dev-dependencies and add clap

9c9c9e4

src: error: Add new errors and impl

a7731a6

RaulTrombin added 4 commits April 25, 2024 16:00

src: lib: Add device module

d26bd27

src: device: Create device module with ping common and inherithed met…

dd45a40

…hods structure

examples: Add ping_1d example using library

df1a1fa

README: Add example instructions

373a6ec

RaulTrombin force-pushed the creave_device_inherit branch from 9f9861b to 373a6ec Compare April 25, 2024 19:02

RaulTrombin requested a review from patrickelectric April 25, 2024 19:03

patrickelectric approved these changes Apr 25, 2024

View reviewed changes

patrickelectric merged commit 68203e7 into bluerobotics:master Apr 25, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add: Device module and example for Ping1D #23

Add: Device module and example for Ping1D #23

RaulTrombin commented Apr 18, 2024

joaoantoniocardoso Apr 24, 2024

RaulTrombin Apr 25, 2024 •

edited

Loading

patrickelectric Apr 25, 2024

RaulTrombin Apr 25, 2024

patrickelectric Apr 25, 2024

RaulTrombin Apr 25, 2024

patrickelectric Apr 25, 2024

patrickelectric Apr 25, 2024

patrickelectric Apr 25, 2024

RaulTrombin Apr 25, 2024 •

edited

Loading

patrickelectric Apr 25, 2024

patrickelectric Apr 25, 2024

RaulTrombin Apr 25, 2024

patrickelectric Apr 25, 2024

patrickelectric Apr 25, 2024

RaulTrombin Apr 25, 2024

patrickelectric Apr 25, 2024

patrickelectric Apr 25, 2024

RaulTrombin commented Apr 25, 2024

		if !(common::AckStruct::id() == answer.message_id
		\|\| common::NackStruct::id() == answer.message_id)

Add: Device module and example for Ping1D #23

Add: Device module and example for Ping1D #23

Conversation

RaulTrombin commented Apr 18, 2024

Choose a reason for hiding this comment

RaulTrombin Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RaulTrombin Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RaulTrombin commented Apr 25, 2024

RaulTrombin Apr 25, 2024 •

edited

Loading

RaulTrombin Apr 25, 2024 •

edited

Loading