[RSDK-9244] [RSDK-9245] Feature gate local signaling and adjust max connections #362

acmorrow · 2024-12-10T22:28:41Z

See notes inline for more details on each change.

…onns

acmorrow · 2024-12-10T22:31:10Z

examples/esp-idf-component/sdkconfig.defaults

@@ -4,6 +4,7 @@ CONFIG_ESP32_DEFAULT_CPU_FREQ_240=y
 CONFIG_ESP_SYSTEM_EVENT_TASK_STACK_SIZE=4096
 CONFIG_PTHREAD_TASK_STACK_SIZE_DEFAULT=8192
 CONFIG_ESP_TLS_SERVER=y
+CONFIG_LWIP_MAX_SOCKETS=13


Now that we have a fix for https://viam.atlassian.net/browse/RSDK-9213 in via #355, I'm more comfortable bumping this up a little bit. Arguably, we can have more sockets now since you may have both local WebRTC and app brokered running simultaneously.

acmorrow · 2024-12-10T22:32:54Z

micro-rdk-ffi/src/ffi/runtime.rs

-            } else {
-                3
-            }
+    #[cfg(target_os = "espidf")]


Logic to determine the number of max conns was present everywhere that led to instantiating a ViamServerBuilder, and was slightly different everywhere. I've opted to consolidate the logic inside ViamServerBuilder and let it decide the default. Explicit calls to with_max_concurrent_connection are no longer required to raise the default above one.

acmorrow · 2024-12-10T22:33:28Z

micro-rdk/Cargo.toml

@@ -15,7 +15,7 @@ links = "micro_rdk"
 crate-type = ["lib"]

 [features]
-default = ["builtin-components", "data"]
+default = ["builtin-components", "data", "local-signaling"]


Added a default-enabled feature gate for local-signaling so it is easier to disable if we need.

acmorrow · 2024-12-10T22:33:52Z

micro-rdk/src/common/conn/viam.rs

@@ -194,6 +194,33 @@ where
    <Storage as RobotConfigurationStorage>::Error: Debug,
    ServerError: From<<Storage as RobotConfigurationStorage>::Error>,
 {
+
+    pub fn get_default_max_concurrent_connections() -> usize {


This is now where we compute the default max number of concurrent connections.

acmorrow · 2024-12-10T22:44:17Z

micro-rdk/src/common/grpc.rs

@@ -145,6 +145,7 @@ where
    }

    // TODO(RSDK-9242): Use the builder pattern instead.
+    #[allow(dead_code)]


I opted not to attempt to actually avoid compiling the local signaling support when the local-signaling feature was off, because it got really messy with the Either type over the app signaling vs local signaling. I doubt the code savings are significant. If anyone thinks it would be worth it, I'm happy to file a ticket for sometime down the road to attempt it.

does this mean we can't complie if local-signaling flag is disabled?

This feels like a weird state to leave things in, and I personally think we should not have a flag for local signaling anyway. If anything, we can have an env var to check if local signaling is disabled and adds/removes it in the builder step.

No, this is exactly so we can compile / clippy when the local-signaling flag is disabled. Otherwise we get warnings here that register_signaling_server is unused.

gotcha, thanks for clarifying.

mattjperez

LGTM, just some questions.

mattjperez · 2024-12-11T18:07:33Z

micro-rdk/src/common/conn/viam.rs

+            max += 2;
+        }
+
+        // But on an esp32 lacking SPIRAM, assume only one connection can be realized


What's the behavior of an SDK trying to connect if local signaling is enabled but lacking spiram (when max connections is 1)?

might be good to have a log message or something to give the user a heads-up about the limitation.
log::warn!("this device can only support one connection at a time blah blah blah");

Yes, actually, you've caught something that I intended to do, but then forgot to execute on. My intention was that if max conns is too small, to disable local signaling, with a warning.

I will add logic for that and post an update.

@mattjperez - Fixed this in latest.

mattjperez · 2024-12-11T18:08:03Z

micro-rdk/src/common/conn/viam.rs

@@ -205,7 +232,7 @@ where
            http2_server_port: 12346,
            http2_server_insecure: false,
            app_client_tasks: Default::default(),
-            max_concurrent_connections: 1,
+            max_concurrent_connections: Self::get_default_max_concurrent_connections(),


mattjperez · 2024-12-11T18:34:40Z

micro-rdk/src/common/grpc.rs

@@ -145,6 +145,7 @@ where
    }

    // TODO(RSDK-9242): Use the builder pattern instead.
+    #[allow(dead_code)]


does this mean we can't complie if local-signaling flag is disabled?

This feels like a weird state to leave things in, and I personally think we should not have a flag for local signaling anyway. If anything, we can have an env var to check if local signaling is disabled and adds/removes it in the builder step.

acmorrow · 2024-12-11T19:13:36Z

micro-rdk/src/common/grpc.rs

@@ -145,6 +145,7 @@ where
    }

    // TODO(RSDK-9242): Use the builder pattern instead.
+    #[allow(dead_code)]


No, this is exactly so we can compile / clippy when the local-signaling flag is disabled. Otherwise we get warnings here that register_signaling_server is unused.

acmorrow · 2024-12-11T19:15:20Z

micro-rdk/src/common/conn/viam.rs

+            max += 2;
+        }
+
+        // But on an esp32 lacking SPIRAM, assume only one connection can be realized


Yes, actually, you've caught something that I intended to do, but then forgot to execute on. My intention was that if max conns is too small, to disable local signaling, with a warning.

I will add logic for that and post an update.

acmorrow · 2024-12-11T19:17:50Z

micro-rdk/src/common/conn/viam.rs

            ),
            robot_config: &config,
-            local_signaling_server: Arc::new(SignalingServer::new(
+            #[cfg(feature = "local-signaling")]
+            local_signaling_server: Some(Arc::new(SignalingServer::new(


@mattjperez - Here is where we should decide to not instantiate the SignalingServer if the max conns is too low.

mattjperez · 2024-12-12T17:19:50Z

micro-rdk/src/common/conn/viam.rs

+        let ss = self
+            .local_signaling_server
+            .clone()
+            .filter(|_| self.incomming_connection_manager.max_connections() >= 5);


that's pretty cool, forgot options can use filter. this way the max_connections() check only happens if self.local_signaling_server.is_some().

My instinct would have been to go with this

let ss = if self.incomming_connection_manager.max_connections() >= 5 { self.local_signaling_server.clone() } else { None }

Did you still want to add the warning?
It's fine if not though

Hah. I actually did have the warning, and then dropped it out after testing. I'll add it back before merging.

[RSDK-9244] [RSDK-9245] Feature gate local signaling and adjust max c…

6187f62

…onns

acmorrow requested review from gvaradarajan and mattjperez December 10, 2024 22:28

acmorrow requested a review from a team as a code owner December 10, 2024 22:28

acmorrow commented Dec 10, 2024

View reviewed changes

format

d8b615e

mattjperez approved these changes Dec 11, 2024

View reviewed changes

acmorrow commented Dec 11, 2024

View reviewed changes

review feedback

a938b63

acmorrow requested a review from mattjperez December 12, 2024 16:49

Add missed commit chunk

7ac43d6

mattjperez approved these changes Dec 12, 2024

View reviewed changes

acmorrow added 2 commits December 12, 2024 18:44

log when disabling

50b813c

Merge branch 'main' into RSDK-9244-9245-local-sig-max-conn

06c9432

acmorrow merged commit 7647cdf into viamrobotics:main Dec 13, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RSDK-9244] [RSDK-9245] Feature gate local signaling and adjust max connections #362

[RSDK-9244] [RSDK-9245] Feature gate local signaling and adjust max connections #362

acmorrow commented Dec 10, 2024

acmorrow Dec 10, 2024

acmorrow Dec 10, 2024

acmorrow Dec 10, 2024

acmorrow Dec 10, 2024

acmorrow Dec 10, 2024

mattjperez Dec 11, 2024

acmorrow Dec 11, 2024

mattjperez Dec 11, 2024

mattjperez left a comment

mattjperez Dec 11, 2024

acmorrow Dec 11, 2024

acmorrow Dec 12, 2024

mattjperez Dec 11, 2024

mattjperez Dec 11, 2024

acmorrow Dec 11, 2024

acmorrow Dec 11, 2024

acmorrow Dec 11, 2024

mattjperez Dec 12, 2024 •

edited

Loading

acmorrow Dec 12, 2024

[RSDK-9244] [RSDK-9245] Feature gate local signaling and adjust max connections #362

[RSDK-9244] [RSDK-9245] Feature gate local signaling and adjust max connections #362

Conversation

acmorrow commented Dec 10, 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

mattjperez left a 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

mattjperez Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattjperez Dec 12, 2024 •

edited

Loading