Skip to content

Conversation

@mrlambchop
Copy link
Contributor

No description provided.

Copilot AI review requested due to automatic review settings December 4, 2025 18:53
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a new overlay to configure Qualcomm modem support for the Tachyon device, including ModemManager configuration, rmtfs service setup, and modem partition management. The overlay is integrated into the ubuntu-common-24.04 stack.

Key changes:

  • New fix-qcom-modem overlay that configures ModemManager, rmtfs service, and modem partition mounts
  • Systemd service overrides for ModemManager and rmtfs to ensure proper initialization order and debugging
  • Comprehensive documentation added in CLAUDE.md describing the overlay system architecture

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
stacks/ubuntu-common-24.04.json Adds the fix-qcom-modem overlay to the build stack
overlays/fix-qcom-modem/overlay.json Main overlay configuration defining modem setup commands
overlays/fix-qcom-modem/files/rmtfs-override.conf Systemd override for rmtfs service configuration
overlays/fix-qcom-modem/files/fstab-entries Filesystem mount entries for modem partitions (/persist and /firmware)
overlays/fix-qcom-modem/files/ModemManager-override.conf Systemd override for ModemManager with debugging and dependencies
overlays/fix-qcom-modem/files/99-tachyon-ignore-at.conf ModemManager filter to ignore specific AT command interfaces
overlays/fix-qcom-modem/create-modem-symlinks.sh Script to create modem partition symlinks in /persist directory
CLAUDE.md Comprehensive documentation of the overlay system, architecture, and usage

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +37 to +41
"cmd": "mkdir -p /persist/rmts"
},
{
"type": "chroot-cmd",
"cmd": "chmod 755 /persist/rmts"
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in directory name: rmts should be rmtfs. Same issue as line 37 - this inconsistency will cause the rmtfs service to fail.

Suggested change
"cmd": "mkdir -p /persist/rmts"
},
{
"type": "chroot-cmd",
"cmd": "chmod 755 /persist/rmts"
"cmd": "mkdir -p /persist/rmtfs"
},
{
"type": "chroot-cmd",
"cmd": "chmod 755 /persist/rmtfs"

Copilot uses AI. Check for mistakes.
"cmd": "chmod 755 /persist/rmts"
},

// Create modem partition symlinks in /persist/rmts
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment references /persist/rmts but should be /persist/rmtfs to match the rmtfs service requirements.

Suggested change
// Create modem partition symlinks in /persist/rmts
// Create modem partition symlinks in /persist/rmtfs

Copilot uses AI. Check for mistakes.
#!/bin/bash
set -euo pipefail

# Create modem partition symlinks in /persist/rmts
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment references /persist/rmts but should be /persist/rmtfs to match the rmtfs service requirements.

Suggested change
# Create modem partition symlinks in /persist/rmts
# Create modem partition symlinks in /persist/rmtfs

Copilot uses AI. Check for mistakes.
Comment on lines +4 to +5
# Create modem partition symlinks in /persist/rmts
cd /persist/rmts
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Directory path uses rmts but should be rmtfs to match the rmtfs service directory structure.

Suggested change
# Create modem partition symlinks in /persist/rmts
cd /persist/rmts
# Create modem partition symlinks in /persist/rmtfs
cd /persist/rmtfs

Copilot uses AI. Check for mistakes.
Comment on lines 26 to 27
"cmd": "grep -q '# Tachyon modem support' /etc/fstab || cat files/fstab-entries >> /etc/fstab",
"ignore-errors": true
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ignore-errors flag is set to true, which could mask genuine errors. If the fstab append fails for reasons other than the entry already existing, the error will be silently ignored. Consider removing this flag and relying on the grep check to prevent duplicate entries, or add more specific error handling.

Suggested change
"cmd": "grep -q '# Tachyon modem support' /etc/fstab || cat files/fstab-entries >> /etc/fstab",
"ignore-errors": true
"cmd": "grep -q '# Tachyon modem support' /etc/fstab || cat files/fstab-entries >> /etc/fstab"

Copilot uses AI. Check for mistakes.
Comment on lines 25 to 26
"type": "chroot-cmd",
"cmd": "grep -q '# Tachyon modem support' /etc/fstab || cat files/fstab-entries >> /etc/fstab",
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The command uses cat files/fstab-entries but commands execute in the chroot environment. The files/fstab-entries path is relative and won't exist inside the chroot. This file needs to be copied into the chroot first, or the command should reference an absolute path that has been copied in earlier.

Consider copying the fstab-entries file into the chroot first (e.g., to /tmp/fstab-entries), then use that path in the cat command:

{
  "type": "copy-into-chroot",
  "source": "files/fstab-entries",
  "destination": "/tmp/fstab-entries",
  "permissions": "644"
},
{
  "type": "chroot-cmd",
  "cmd": "grep -q '# Tachyon modem support' /etc/fstab || cat /tmp/fstab-entries >> /etc/fstab",
  "ignore-errors": true
}
Suggested change
"type": "chroot-cmd",
"cmd": "grep -q '# Tachyon modem support' /etc/fstab || cat files/fstab-entries >> /etc/fstab",
"type": "copy-into-chroot",
"source": "files/fstab-entries",
"destination": "/tmp/fstab-entries",
"permissions": "644"
},
{
"type": "chroot-cmd",
"cmd": "grep -q '# Tachyon modem support' /etc/fstab || cat /tmp/fstab-entries >> /etc/fstab",

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +16
[Service]
RestrictAddressFamilies=
RestrictAddressFamilies=AF_NETLINK AF_UNIX AF_QIPCRTR AF_INET AF_INET6

Environment=MM_PLUGIN_DEBUG=qcom-soc
ExecStart=
ExecStart=/usr/sbin/ModemManager --debug

[Unit]
# Ensure rmtfs is up before ModemManager starts
After=rmtfs.service
Requires=rmtfs.service

[Service]
# Add a delay to ensure the modem has initialized and QRTR is populated
ExecStartPre=/bin/sleep 10
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The [Service] section appears twice in this systemd override file (lines 1 and 14). While systemd will merge these sections, it's a best practice to consolidate all Service directives under a single [Service] section for clarity and maintainability. Consider restructuring as:

[Unit]
After=rmtfs.service
Requires=rmtfs.service

[Service]
RestrictAddressFamilies=
RestrictAddressFamilies=AF_NETLINK AF_UNIX AF_QIPCRTR AF_INET AF_INET6
Environment=MM_PLUGIN_DEBUG=qcom-soc
ExecStartPre=/bin/sleep 10
ExecStart=
ExecStart=/usr/sbin/ModemManager --debug

Copilot uses AI. Check for mistakes.
},
{
"type": "chroot-cmd",
"cmd": "mkdir -p /persist/rmts"
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in directory name: rmts should be rmtfs (Remote File System service). This is inconsistent with the service name rmtfs and could cause the service to fail since it expects data in /persist/rmtfs not /persist/rmts.

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,3 @@
[Service]
ExecStart=
ExecStart=/usr/bin/rmtfs -P -o /persist/rmts -s -v
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The -o path parameter uses /persist/rmts but should be /persist/rmtfs to match the standard rmtfs directory structure.

Copilot uses AI. Check for mistakes.
Comment on lines +30 to +42
// Set up /persist directory structure and permissions
{
"type": "chroot-cmd",
"cmd": "chmod 755 /persist"
},
{
"type": "chroot-cmd",
"cmd": "mkdir -p /persist/rmts"
},
{
"type": "chroot-cmd",
"cmd": "chmod 755 /persist/rmts"
},
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting permissions on /persist before it's mounted could be ineffective. The /persist directory is created at line 20, but according to the fstab entries, /dev/sdf8 should be mounted to /persist. The chmod operations at lines 33 and 41 will apply to the empty mount point, not the mounted filesystem. These permission changes should occur after the partitions are mounted, or they should be part of a post-mount initialization script.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants