Initial support 1.5.8 #40

evshary · 2024-06-03T13:19:17Z

The PR is to support up-spec 1.5.8
Close #39

Signed-off-by: ChenYing Kuo <evshary@gmail.com>

evshary · 2024-06-04T11:12:23Z

@PLeVasseur Would you mind taking a look?

PLeVasseur · 2024-06-04T21:45:24Z

Hi @evshary -- I'm on vacation starting tomorrow till next Monday. I'll see if I can sneak a review of this in. :)

evshary · 2024-06-05T02:14:23Z

Hi @evshary -- I'm on vacation starting tomorrow till next Monday. I'll see if I can sneak a review of this in. :)

Hi @PLeVasseur It's fine to be reviewed next week. Enjoy your vacation!

Signed-off-by: ChenYing Kuo <evshary@gmail.com>

PLeVasseur · 2024-06-11T22:14:24Z

src/lib.rs

@@ -31,6 +29,15 @@ use zenoh::{
    subscriber::Subscriber,
 };

+// CY_TODO: Whether to expose from up_rust or not


Very good question. IMHO it makes sense for these to live in up-rust.

PLeVasseur · 2024-06-11T22:25:49Z

src/lib.rs

@@ -277,97 +208,111 @@ impl UPClientZenoh {
        };
        Ok(uattributes)
    }
+
+    /*        The table for mapping resource ID to message type


If this table doesn't exist in up-spec it'd be great to have you contribute to adding it there!

So is the table clear to you? The reason why I hesitate to add the table into spec is that I'm afraid it makes readers difficult to understand.

Yeah, I get it. I spent a few minutes just mentally going over the table to see if it aligned with my understanding.

I think, even if it takes some time for someone to process what's in the table, it makes a lot more sense to have it written up in a place like up-spec that can be referenced for any implementation 🙂

PLeVasseur · 2024-06-12T01:49:41Z

src/utransport.rs

-            self.register_request_listener(&topic, listener.clone())
+        let flag = UPClientZenoh::get_listener_message_type(source_filter, sink_filter)?;
+        // Publish & Notification
+        if flag.contains(MessageFlag::Publish) || flag.contains(MessageFlag::Notification) {


So I think based on how I am reading this, the intention looks to be that the source and sink filters may result in more than one bit being set in the bitmask enum. That seems to be the intent. And that makes sense.

I'm having a hard time seeing how the function get_listener_message_type() can set multiple bits. It looks like a single value is set based on the if conditionals and returned. Hmm. Perhaps I need to read more 🤔

You're right... I made the silly mistakes.
I updated how get_listener_message_type works and added more tests case to avoid the mistakes in the future.

PLeVasseur · 2024-06-12T02:01:43Z

tests/register.rs

-#[test_case(test_lib::create_utransport_uuri(Some(0), 0, 0); "Publish / Notification register_listener")]
-#[test_case(test_lib::create_rpcserver_uuri(Some(0), 0); "RPC register_listener")]
-#[test_case(test_lib::create_special_uuri(0); "Special UUri register_listener")]
+#[test_case(&test_lib::new_uuri("src", 1, 1, 0x8000), None; "Listen to Publish")]


Is there a way to listen to all point to point messages? I see ability to listen to all messages of a given type. What about being able to express "all messages with a sink whose source authority_name is foo"?

That's one of the streamer use cases for connecting two authorities together over two transports.

Just checked the uP-L1 Zenoh spec. Looks like this case may have been missed. Can we update the spec to clarify this is also possible and add this capability to the transport? (If I am reading it correctly it looks like registering to get all point to point messages as it was in 1.5.7 is not possible in the current implementation. Very possible I'm wrong and if so please point it out 😅)

Is this what you expect? I added the uStreamer test cases in the tests now.
ea3a993#diff-c19a9c920bb76a5ab06a7d28f16df025d34c3a66697465b9e92c9df37ecdc0a3R36

I think this is pretty close, left a comment on that test.

PLeVasseur · 2024-06-12T02:08:18Z

src/lib.rs

+     *  - Response: {(0-8000), 0}, {(0-8000), FFFF}, (FFFF, 0), {FFFF, FFFF}
+     */
+    #[allow(clippy::nonminimal_bool)] // Don't simplify the boolean expression for better understanding
+    fn get_listener_message_type(


IMHO something very similar could be useful in up-rust with modifications to support the uStreamer case of all point to point messages from a certain authority.

Signed-off-by: ChenYing Kuo <evshary@gmail.com>

PLeVasseur

I think you may need to modify or make a new test so that we can test the uStreamer use-case of all of Notification, Request, and Response.

PLeVasseur · 2024-06-12T23:49:21Z

I think it'd be good to update the name to up-transport-zenoh, too.

PLeVasseur · 2024-06-13T00:20:29Z

As I start to work with the Zenoh transport, I think it would be good if from this crate we re-exported some key Zenoh items such that an end-user of up-transport-zenoh-rust doesn't also need to pull in Zenoh as a dependency and can use the stuff required from there.

I'm thinking specifically at the moment about Zenoh's Config, as it would appear in order to configure the Zenoh transport I do need to pass in a Config into new().

src/lib.rs

Signed-off-by: ChenYing Kuo <evshary@gmail.com>

evshary · 2024-06-13T08:51:54Z

I think you may need to modify or make a new test so that we can test the uStreamer use-case of all of Notification, Request, and Response.

I found something wrong with Response use case. In the previous implementation, calling register_listener for Response message doesn't work with a wildcard. It always needs to have the fully-matching Zenoh key. That means the Zenoh key from register_listener and send(request) should be the same.

I fixed the issue with the help of the intersection in Zenoh key. Now register_listener and send(request) don't need to be the same. They can be intersected.

@PLeVasseur Let me know if the test case is not what you imagine.

PLeVasseur · 2024-06-13T14:01:04Z

src/lib.rs

-            entity: Some(uentity).into(),
-            ..Default::default()
-        };
+    pub async fn new(config: Config, authority_name: String) -> Result<UPClientZenoh, UStatus> {


Hey @evshary -- for the uStreamer use-case we'll need a function to build a UPClientZenoh object from a Zenoh Runtime in order to fit within the topology with up-linux-streamer as a Zenoh plugin.

I'm okay if you want to merge this one and address that in a follow up. 👍

OK. Don't make the PR to lengthy. I'll create an issue to track.

PLeVasseur · 2024-06-13T14:02:58Z

src/lib.rs

@@ -21,8 +21,9 @@ use std::{
 };
 use tokio::runtime::Runtime;
 use up_rust::{ComparableListener, UAttributes, UCode, UListener, UPriority, UStatus, UUri};
+// Re-export Zenoh config
+pub use zenoh::config::Config;


Is this enough to configure a Config completely? Or should we re-export all of zenoh::config::*?

It seems like there will be some conflicts if expose all config. I'll deal with it later.

Initial support 1.5.8

9a773f2

Signed-off-by: ChenYing Kuo <evshary@gmail.com>

evshary force-pushed the bump_1.5.8 branch from 4dfb00c to b5d6c08 Compare June 4, 2024 03:58

fix building error on CI.

d403920

Signed-off-by: ChenYing Kuo <evshary@gmail.com>

evshary force-pushed the bump_1.5.8 branch from b5d6c08 to d403920 Compare June 4, 2024 04:00

evshary added 4 commits June 4, 2024 18:23

Add more tests for Zenoh key transformation.

4039426

Signed-off-by: ChenYing Kuo <evshary@gmail.com>

Remove the encoding.

155a9cf

Signed-off-by: ChenYing Kuo <evshary@gmail.com>

Fix eclipse-uprotocol#36 return error.

c00ec33

Signed-off-by: ChenYing Kuo <evshary@gmail.com>

Update how to convert to Value

70e45ba

Signed-off-by: ChenYing Kuo <evshary@gmail.com>

evshary marked this pull request as ready for review June 4, 2024 11:08

evshary linked an issue Jun 4, 2024 that may be closed by this pull request

unregister_listener() when listener isn't registered to topic already #36

Closed

evshary linked an issue Jun 4, 2024 that may be closed by this pull request

Flesh out Zenoh key conversion tests and align with up-client-zenoh-cpp #23

Closed

evshary mentioned this pull request Jun 4, 2024

Flesh out Zenoh key conversion tests and align with up-client-zenoh-cpp #23

Closed

evshary mentioned this pull request Jun 5, 2024

Update the examples to match 1.5.8 eclipse-uprotocol/up-zenoh-example-rust#7

Closed

Update the order of Cargo.toml.

9c6742c

Signed-off-by: ChenYing Kuo <evshary@gmail.com>

PLeVasseur reviewed Jun 11, 2024

View reviewed changes

PLeVasseur reviewed Jun 12, 2024

View reviewed changes

Fix the get_listener_message_type and add more test cases.

ea3a993

Signed-off-by: ChenYing Kuo <evshary@gmail.com>

PLeVasseur requested changes Jun 12, 2024

View reviewed changes

PLeVasseur reviewed Jun 13, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

evshary added 3 commits June 13, 2024 11:30

Rename the project.

fb97deb

Signed-off-by: ChenYing Kuo <evshary@gmail.com>

Re-export the Config.

08363f8

Signed-off-by: ChenYing Kuo <evshary@gmail.com>

Support uStreamer test.

e23e6c2

Signed-off-by: ChenYing Kuo <evshary@gmail.com>

PLeVasseur reviewed Jun 13, 2024

View reviewed changes

evshary merged commit b8925c6 into eclipse-uprotocol:main Jun 14, 2024
7 checks passed

evshary deleted the bump_1.5.8 branch June 14, 2024 02:36

This was referenced Jun 14, 2024

Support initializing with Zenoh Runtime #42

Closed

Update the up-spec with the table in the comments #45

Closed

Add the table for listeners to distinguish different message types. eclipse-uprotocol/up-spec#188

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial support 1.5.8 #40

Initial support 1.5.8 #40

evshary commented Jun 3, 2024

evshary commented Jun 4, 2024

PLeVasseur commented Jun 4, 2024

evshary commented Jun 5, 2024

PLeVasseur Jun 11, 2024

PLeVasseur Jun 11, 2024

evshary Jun 12, 2024

PLeVasseur Jun 12, 2024

PLeVasseur Jun 12, 2024

evshary Jun 12, 2024

PLeVasseur Jun 12, 2024

PLeVasseur Jun 12, 2024

evshary Jun 12, 2024

PLeVasseur Jun 12, 2024

PLeVasseur Jun 12, 2024

PLeVasseur left a comment •

edited

Loading

PLeVasseur commented Jun 12, 2024

PLeVasseur commented Jun 13, 2024

evshary commented Jun 13, 2024

PLeVasseur Jun 13, 2024

evshary Jun 14, 2024

PLeVasseur Jun 13, 2024

evshary Jun 14, 2024

Initial support 1.5.8 #40

Initial support 1.5.8 #40

Conversation

evshary commented Jun 3, 2024

evshary commented Jun 4, 2024

PLeVasseur commented Jun 4, 2024

evshary commented Jun 5, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PLeVasseur left a comment • edited Loading

Choose a reason for hiding this comment

PLeVasseur commented Jun 12, 2024

PLeVasseur commented Jun 13, 2024

evshary commented Jun 13, 2024

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

PLeVasseur left a comment •

edited

Loading