-
-
Notifications
You must be signed in to change notification settings - Fork 8
added back respeaker drivers #140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
WalkthroughThe updates modify the respeaker driver installation process in the build script by invoking a dedicated setup script, which now dynamically selects the appropriate driver version based on the running kernel. Commented-out update commands are cleaned up, and the setup script adds logic to report installation success or failure. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/setup_respeaker.sh (1)
21-26: Quote variables and explicitly fallback to default branch
Unquoted$checkout_versioncan break on unusual tag names. Also, while failing a checkout leaves you on the original branch, it’s clearer to explicitlygit checkoutthe default (e.g.,mainormaster) to avoid a detached HEAD state.Example:
- if git checkout $checkout_version; then + if git checkout --quiet "$checkout_version"; then echo "using version $checkout_version of respeaker drivers" else echo "Could not checkout version $checkout_version" + git checkout main # or `master`, whichever is the default echo "Falling back to default branch" fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
build_raspOVOS.sh(1 hunks)scripts/setup_respeaker.sh(3 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/setup_respeaker.sh
[error] 67-67: Couldn't parse this test expression. Fix to allow more checks.
(SC1073)
[error] 67-67: Expected comparison operator (don't wrap commands in []/[[]]). Fix any mentioned problems and try again.
(SC1072)
🔇 Additional comments (1)
build_raspOVOS.sh (1)
146-148: Properly invoke the ReSpeaker setup script
The addition of the echo andbash setup_respeaker.shcommands correctly integrates driver installation into the build workflow, leveragingset -eto abort on failure.
| kernel=$(uname -r) | ||
| k_ver=$(echo $kernel | cut -d '.' -f2) | ||
| checkout_version="v6.$k_ver" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve kernel version extraction logic
Current code only captures the minor version and hardcodes major as “6”, which will break on non-6.x kernels and may mis-handle tags. Also, variable expansions should be quoted to prevent word-splitting.
Proposed diff:
- kernel=$(uname -r)
- k_ver=$(echo $kernel | cut -d '.' -f2)
- checkout_version="v6.$k_ver"
+ # Extract major and minor from the raw kernel version (ignore suffix)
+ kernel_base=$(uname -r | cut -d '-' -f1)
+ major=$(echo "$kernel_base" | cut -d '.' -f1)
+ minor=$(echo "$kernel_base" | cut -d '.' -f2)
+ checkout_version="v${major}.${minor}"This makes the tag dynamic and robust across kernel series.
🤖 Prompt for AI Agents
In scripts/setup_respeaker.sh around lines 17 to 19, the kernel version
extraction hardcodes the major version as "6" and only extracts the minor
version, which breaks compatibility with other kernel series. Update the code to
dynamically extract both the major and minor kernel version parts from the uname
output, and quote all variable expansions to prevent word splitting. Construct
the checkout_version variable using these extracted parts to make the tag
dynamic and robust across different kernel versions.
Updates script to use either the version of current kernel, or the default branch for the respeaker drivers
Summary by CodeRabbit
New Features
Bug Fixes