Merge from upstream #1
Conversation
This option will automatically report the latest BELABOX stats in the chat
* Fix: Add ModemStatus handling to StatusKind enum and match statement
These fields were accidentally removed in an older commit.
Also fixes some message fields
WalkthroughThis set of changes introduces substantial enhancements to modem and network interface monitoring, including new fields and message variants for tracking modem status, relay servers, and available updates. Several data structures are made more flexible by making fields optional. The command and monitoring subsystems are extended to support a new modem status command and periodic network status reporting. Documentation and dependency versions are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Bot
participant CommandHandler
participant BelaState
participant Monitor
User->>Bot: Sends chat command (!bbm)
Bot->>CommandHandler: Dispatches BotCommand::Modems
CommandHandler->>BelaState: Reads modem and interface state
CommandHandler-->>Bot: Returns modem status string
Bot-->>User: Replies with modem status
Note over Monitor,CommandHandler: Periodically (if enabled)
Monitor->>BelaState: Checks streaming and timeout
Monitor->>CommandHandler: Gets stats (network status)
CommandHandler-->>Monitor: Returns stats string
Monitor-->>User: Sends periodic network status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/belabox/requests.rs (1)
96-115: Unit-test no longer compiles after theOption<T>transition
asrc,acodec,relay_server,relay_account, and the SRT fields are nowOption<T>but the test still passes bareStrings and omits the new optional fields.- asrc: "No audio".to_string(), - acodec: "opus".to_string(), - remote_key: "remote_key".to_string(), - relay_server: "1".to_string(), - relay_account: "1".to_string(), + asrc: Some("No audio".to_string()), + acodec: Some("opus".to_string()), + remote_key: "remote_key".to_string(), + relay_server: Some("1".to_string()), + relay_account: Some("1".to_string()), + srt_streamid: None, + srtla_addr: None, + srtla_port: None,Without this fix the crate will fail to compile.
🧹 Nitpick comments (8)
src/config.rs (1)
39-43: Clarify units and guardrails for network_timeoutConsider documenting that
network_timeoutis in seconds and validating/clamping unreasonable values (e.g., 0) at load time to prevent spammy behavior.src/belabox.rs (3)
151-151: Redundant .into() on String
auth_requestis already aString. Remove.into()for clarity.- if let Err(e) = write.send(TMessage::Text(auth_request.into())).await { + if let Err(e) = write.send(TMessage::Text(auth_request)).await {
214-217: Redundant .into() on String in keepalive
serde_json::to_string(...).unwrap()returnsString. No need for.into().- .send(TMessage::Text( - serde_json::to_string(&Request::Keepalive(None)) - .unwrap() - .into(), - )) + .send(TMessage::Text( + serde_json::to_string(&Request::Keepalive(None)).unwrap(), + ))
315-315: Redundant .into() on String in request sender
request.messageis already aString.- .send(TMessage::Text(request.message.into())) + .send(TMessage::Text(request.message))README.md (2)
25-29: Docs: new monitor fields captured; add explanations belowYou added
networkandnetwork_timeoutto the example. Consider adding brief bullets in the BELABOX/monitor section explaining:
network: enable periodic network status messagesnetwork_timeout: minimum seconds between messages while streaming
140-141: Clarify Stats vs Modems descriptions and keep examples consistentBoth rows currently claim to show “connected modems status and bitrate.” Based on code:
Statsreports per-interface bitrate (+ UPS), not modem statusModemsreports modem details (type, network, connection, signal, roaming)Please adjust descriptions and consider adding a “Modems” entry to the commands JSON example above for consistency.
src/monitor.rs (1)
69-82: Redundant string allocation innetif_name
netif_nameclones the key three times and allocates a newStringeven when no rename occurs.
A simple&strreturn (orCow<'_, str>) would avoid the extra heap allocation on every iteration.src/belabox/messages.rs (1)
124-148: Key matching is case-sensitive – camelCase variants are ignoredThe deserializer recognises
"available_updates"but not"availableUpdates","isStreaming"etc.
If firmware switches to camelCase the message will silently fall back to the bulkyStatusvariant.Consider normalising the key or matching both styles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.toml(2 hunks)README.md(2 hunks)src/belabox.rs(4 hunks)src/belabox/messages.rs(8 hunks)src/belabox/requests.rs(3 hunks)src/bot.rs(9 hunks)src/command_handler.rs(5 hunks)src/config.rs(4 hunks)src/monitor.rs(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/command_handler.rs (2)
src/monitor.rs (2)
modems(62-112)network(179-199)src/bot.rs (1)
new(64-111)
src/bot.rs (1)
src/config.rs (1)
default(46-56)
🔇 Additional comments (10)
src/config.rs (4)
39-43: Monitor: new network fields are backward-compatible and sensible defaultsAdding
network: boolandnetwork_timeout: u64under a#[serde(default)]-ed struct, with aDefaultimpl, preserves compatibility with existing configs.
51-55: Default values align with README
network: falseandnetwork_timeout: 30match the README example; no issues found.
79-79: New BotCommand::Modems variant addedThis integrates cleanly with the command dispatcher and default commands. No enum serialization concerns spotted.
289-295: Default command for Modems added; verify intended permissionYou set
!bbmtoBroadcaster. Given thatStatsisPublic, double-check whetherModemsshould also be public (info-only) or restricted as-is.src/belabox.rs (1)
251-251: Improved error logging contextLogging
?textalongside the error is a good improvement for diagnosing deserialization issues.Cargo.toml (1)
3-3: Multiple dependency upgrades; validate for breaking changes
- tokio-tungstenite 0.26 and thiserror 2.x are major upgrades. APIs used here look OK, but please build and run end-to-end to catch subtle breaks (e.g., error variants, CloseFrame types).
- Added serde_with likely for skip-serializing Option::None; ensure code paths using it are covered.
Action items:
- Build and run with feature flags used in production.
- Sanity-check WebSocket connect/auth/send flows at runtime.
Also applies to: 13-16, 20-20
src/command_handler.rs (4)
15-15: Derive Clone for CommandHandlerReasonable given it’s shared across tasks; fields are Arcs or owned collections so clone cost is bounded.
56-57: Command routing: Modems handler wired inDispatch for
BotCommand::Modemslooks correct; matches config and README additions.
182-187: Stats: append total bitrate only when multiple interfacesBehavior change from always appending. Verify this is intended UX—some users may still want a Total even with a single interface.
665-665: audio_src: set Option explicitlyWrapping in
Some(...)is correct given Start/config now treatsasrcas optional. Matches serde_with usage to skip None.
| let current_modems = &mut lock.modems; | ||
|
|
||
| for (key, modem) in incoming.modems.iter() { | ||
| current_modems | ||
| .entry(key.to_string()) | ||
| .and_modify(|existing| { | ||
| existing.status = modem.status.clone(); | ||
| }) | ||
| .or_insert_with(|| modem.clone()); | ||
| } | ||
|
|
||
| // Remove modems that are not present in the incoming update. | ||
| current_modems.retain(|key, _| incoming.modems.contains_key(key)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Only the status field is refreshed – other modem data may go stale
Incoming updates can carry altered ifname, network_type, signal strength, etc., yet the merge only mutates status, leaving older fields untouched.
- .and_modify(|existing| {
- existing.status = modem.status.clone();
- })
+ .and_modify(|existing| *existing = modem.clone())This keeps the cache fully in sync with the BELABOX state.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let current_modems = &mut lock.modems; | |
| for (key, modem) in incoming.modems.iter() { | |
| current_modems | |
| .entry(key.to_string()) | |
| .and_modify(|existing| { | |
| existing.status = modem.status.clone(); | |
| }) | |
| .or_insert_with(|| modem.clone()); | |
| } | |
| // Remove modems that are not present in the incoming update. | |
| current_modems.retain(|key, _| incoming.modems.contains_key(key)); | |
| } | |
| let current_modems = &mut lock.modems; | |
| for (key, modem) in incoming.modems.iter() { | |
| current_modems | |
| .entry(key.to_string()) | |
| - .and_modify(|existing| { | |
| - existing.status = modem.status.clone(); | |
| - }) | |
| + .and_modify(|existing| *existing = modem.clone()) | |
| .or_insert_with(|| modem.clone()); | |
| } | |
| // Remove modems that are not present in the incoming update. | |
| current_modems.retain(|key, _| incoming.modems.contains_key(key)); | |
| } |
🤖 Prompt for AI Agents
In src/bot.rs around lines 163 to 176, the code only updates the `status` field
of existing modems during the merge, which causes other fields like `ifname`,
`network_type`, and signal strength to become stale. To fix this, update the
entire modem entry with the incoming modem data instead of just the `status`
field when modifying existing entries, ensuring the cache stays fully
synchronized with the BELABOX state.
| pub async fn modems(&self) -> Result<String> { | ||
| let (netifs, modems) = { | ||
| let state = self.bela_state.read().await; | ||
| (state.netif.clone(), state.modems.clone()) | ||
| }; | ||
|
|
||
| let modem_infos: Vec<String> = modems | ||
| .values() | ||
| .map(|modem| { | ||
| // Determine the interface name using a custom name if available | ||
| // FIXME: The name could be set once in bot.rs | ||
| let name_label = if let Some(ifname) = &modem.ifname { | ||
| netifs | ||
| .as_ref() | ||
| .and_then(|netifs| netifs.get(ifname)) | ||
| .and_then(|iface| { | ||
| self.custom_interface_name | ||
| .get(ifname) | ||
| .or_else(|| self.custom_interface_name.get(&iface.ip)) | ||
| }) | ||
| .cloned() | ||
| .unwrap_or_else(|| ifname.to_owned()) | ||
| } else { | ||
| "Modem".to_string() | ||
| }; | ||
|
|
||
| // Build status string if available | ||
| let status_info = modem.status.as_ref().map_or(String::new(), |status| { | ||
| let network_type = status.network_type.as_deref().unwrap_or("?G"); | ||
| let network = status.network.as_deref().unwrap_or("Unknown Network"); | ||
|
|
||
| let mut info = format!( | ||
| "{} on {}, {}, signal {}", | ||
| network_type, network, status.connection, status.signal | ||
| ); | ||
| if status.roaming { | ||
| info.push_str(", roaming"); | ||
| } | ||
| info | ||
| }); | ||
|
|
||
| format!("{}: {}", name_label, status_info).trim().to_owned() | ||
| }) | ||
| .collect(); | ||
|
|
||
| Ok(modem_infos.join(" - ")) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Modems: handle empty/no-status cases and avoid dangling colon
- Current formatting can produce “Label:” with no details if status is None, and an empty final message if there are zero modems.
- Tighten message construction to omit the colon when no status, and return a friendly message if there are no modems.
Proposed refactor:
- let modem_infos: Vec<String> = modems
- .values()
- .map(|modem| {
- // Determine the interface name using a custom name if available
- // FIXME: The name could be set once in bot.rs
- let name_label = if let Some(ifname) = &modem.ifname {
- netifs
- .as_ref()
- .and_then(|netifs| netifs.get(ifname))
- .and_then(|iface| {
- self.custom_interface_name
- .get(ifname)
- .or_else(|| self.custom_interface_name.get(&iface.ip))
- })
- .cloned()
- .unwrap_or_else(|| ifname.to_owned())
- } else {
- "Modem".to_string()
- };
-
- // Build status string if available
- let status_info = modem.status.as_ref().map_or(String::new(), |status| {
- let network_type = status.network_type.as_deref().unwrap_or("?G");
- let network = status.network.as_deref().unwrap_or("Unknown Network");
-
- let mut info = format!(
- "{} on {}, {}, signal {}",
- network_type, network, status.connection, status.signal
- );
- if status.roaming {
- info.push_str(", roaming");
- }
- info
- });
-
- format!("{}: {}", name_label, status_info).trim().to_owned()
- })
- .collect();
-
- Ok(modem_infos.join(" - "))
+ let modem_infos: Vec<String> = modems
+ .values()
+ .map(|modem| {
+ // Determine interface display name using a custom name if available
+ let label = if let Some(ifname) = &modem.ifname {
+ netifs
+ .as_ref()
+ .and_then(|ifs| ifs.get(ifname))
+ .and_then(|iface| {
+ self.custom_interface_name
+ .get(ifname)
+ .or_else(|| self.custom_interface_name.get(&iface.ip))
+ })
+ .cloned()
+ .unwrap_or_else(|| ifname.to_owned())
+ } else {
+ "Modem".to_string()
+ };
+
+ if let Some(status) = modem.status.as_ref() {
+ let network_type = status.network_type.as_deref().unwrap_or("?G");
+ let network = status.network.as_deref().unwrap_or("Unknown Network");
+ let mut info = format!(
+ "{} on {}, {}, signal {}",
+ network_type, network, status.connection, status.signal
+ );
+ if status.roaming {
+ info.push_str(", roaming");
+ }
+ format!("{label}: {info}")
+ } else {
+ label
+ }
+ })
+ .filter(|s| !s.is_empty())
+ .collect();
+
+ if modem_infos.is_empty() {
+ return Ok("No modems found".to_string());
+ }
+
+ Ok(modem_infos.join(" - "))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub async fn modems(&self) -> Result<String> { | |
| let (netifs, modems) = { | |
| let state = self.bela_state.read().await; | |
| (state.netif.clone(), state.modems.clone()) | |
| }; | |
| let modem_infos: Vec<String> = modems | |
| .values() | |
| .map(|modem| { | |
| // Determine the interface name using a custom name if available | |
| // FIXME: The name could be set once in bot.rs | |
| let name_label = if let Some(ifname) = &modem.ifname { | |
| netifs | |
| .as_ref() | |
| .and_then(|netifs| netifs.get(ifname)) | |
| .and_then(|iface| { | |
| self.custom_interface_name | |
| .get(ifname) | |
| .or_else(|| self.custom_interface_name.get(&iface.ip)) | |
| }) | |
| .cloned() | |
| .unwrap_or_else(|| ifname.to_owned()) | |
| } else { | |
| "Modem".to_string() | |
| }; | |
| // Build status string if available | |
| let status_info = modem.status.as_ref().map_or(String::new(), |status| { | |
| let network_type = status.network_type.as_deref().unwrap_or("?G"); | |
| let network = status.network.as_deref().unwrap_or("Unknown Network"); | |
| let mut info = format!( | |
| "{} on {}, {}, signal {}", | |
| network_type, network, status.connection, status.signal | |
| ); | |
| if status.roaming { | |
| info.push_str(", roaming"); | |
| } | |
| info | |
| }); | |
| format!("{}: {}", name_label, status_info).trim().to_owned() | |
| }) | |
| .collect(); | |
| Ok(modem_infos.join(" - ")) | |
| } | |
| pub async fn modems(&self) -> Result<String> { | |
| let (netifs, modems) = { | |
| let state = self.bela_state.read().await; | |
| (state.netif.clone(), state.modems.clone()) | |
| }; | |
| let modem_infos: Vec<String> = modems | |
| .values() | |
| .map(|modem| { | |
| // Determine interface display name using a custom name if available | |
| let label = if let Some(ifname) = &modem.ifname { | |
| netifs | |
| .as_ref() | |
| .and_then(|ifs| ifs.get(ifname)) | |
| .and_then(|iface| { | |
| self.custom_interface_name | |
| .get(ifname) | |
| .or_else(|| self.custom_interface_name.get(&iface.ip)) | |
| }) | |
| .cloned() | |
| .unwrap_or_else(|| ifname.to_owned()) | |
| } else { | |
| "Modem".to_string() | |
| }; | |
| if let Some(status) = modem.status.as_ref() { | |
| let network_type = status.network_type.as_deref().unwrap_or("?G"); | |
| let network = status.network.as_deref().unwrap_or("Unknown Network"); | |
| let mut info = format!( | |
| "{} on {}, {}, signal {}", | |
| network_type, network, status.connection, status.signal | |
| ); | |
| if status.roaming { | |
| info.push_str(", roaming"); | |
| } | |
| format!("{label}: {info}") | |
| } else { | |
| label | |
| } | |
| }) | |
| .filter(|s| !s.is_empty()) | |
| .collect(); | |
| if modem_infos.is_empty() { | |
| return Ok("No modems found".to_string()); | |
| } | |
| Ok(modem_infos.join(" - ")) | |
| } |
🤖 Prompt for AI Agents
In src/command_handler.rs between lines 196 and 242, the modems function
currently formats modem info with a colon even when there is no status,
resulting in dangling colons and an empty string if no modems exist. To fix
this, modify the formatting logic to only include the colon and status info if
status is present, otherwise just show the label without a colon. Additionally,
check if the modems collection is empty and return a friendly message like "No
modems found" instead of an empty string. This ensures cleaner output and better
user feedback.
| pub async fn network(&self, network_timeout: u64) { | ||
| { | ||
| let mut lock = self.bela_state.write().await; | ||
| if !lock.is_streaming { | ||
| return; | ||
| } | ||
|
|
||
| let timeout = &mut lock.network_timeout; | ||
| if timeout.elapsed() < Duration::from_secs(network_timeout) { | ||
| return; | ||
| } else { | ||
| *timeout = Instant::now(); | ||
| } | ||
| } | ||
|
|
||
| let lock = self.command_handler.lock().await; | ||
| let Some(ch) = &*lock else { return }; | ||
| let Ok(msg) = ch.stats().await else { return }; | ||
|
|
||
| self.send(msg).await; | ||
| } |
There was a problem hiding this comment.
Hold-off the mutex before an .await to prevent lock contention
self.command_handler.lock().await is kept across the asynchronous ch.stats().await call.
If stats() performs I/O the mutex stays locked for its whole duration, blocking any writer that tries to swap or drop the handler.
- let lock = self.command_handler.lock().await;
- let Some(ch) = &*lock else { return };
- let Ok(msg) = ch.stats().await else { return };
+ // Grab the handler, then release the mutex before awaiting.
+ let ch = {
+ let lock = self.command_handler.lock().await;
+ match &*lock {
+ Some(h) => h.clone(),
+ None => return,
+ }
+ };
+
+ let Ok(msg) = ch.stats().await else { return };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub async fn network(&self, network_timeout: u64) { | |
| { | |
| let mut lock = self.bela_state.write().await; | |
| if !lock.is_streaming { | |
| return; | |
| } | |
| let timeout = &mut lock.network_timeout; | |
| if timeout.elapsed() < Duration::from_secs(network_timeout) { | |
| return; | |
| } else { | |
| *timeout = Instant::now(); | |
| } | |
| } | |
| let lock = self.command_handler.lock().await; | |
| let Some(ch) = &*lock else { return }; | |
| let Ok(msg) = ch.stats().await else { return }; | |
| self.send(msg).await; | |
| } | |
| pub async fn network(&self, network_timeout: u64) { | |
| { | |
| let mut lock = self.bela_state.write().await; | |
| if !lock.is_streaming { | |
| return; | |
| } | |
| let timeout = &mut lock.network_timeout; | |
| if timeout.elapsed() < Duration::from_secs(network_timeout) { | |
| return; | |
| } else { | |
| *timeout = Instant::now(); | |
| } | |
| } | |
| // Grab the handler, then release the mutex before awaiting. | |
| let ch = { | |
| let lock = self.command_handler.lock().await; | |
| match &*lock { | |
| Some(h) => h.clone(), | |
| None => return, | |
| } | |
| }; | |
| let Ok(msg) = ch.stats().await else { return }; | |
| self.send(msg).await; | |
| } |
🤖 Prompt for AI Agents
In src/monitor.rs around lines 179 to 199, the mutex lock on
self.command_handler is held across the asynchronous call ch.stats().await,
which can cause lock contention if stats() performs I/O. To fix this, acquire
the lock, clone or extract the command handler reference needed for stats(),
then release the lock before calling the async stats() method. This ensures the
mutex is not held during the await, preventing blocking other writers.
3 participants
Summary by CodeRabbit
New Features
!bbm) to show connected modems and bitrate.Improvements
Documentation
Dependency Updates