Skip to content

Fixed detection and upload of driver for Mark 2 device #24

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.

Sign up for GitHub

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

Open
builderjer wants to merge 7 commits into
base: dev
Choose a base branch
from
Open

Fixed detection and upload of driver for Mark 2 device #24

Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
50ed58a
Fixed detection and upload of driver for Mark 2 device
builderjer Aug 17, 2025
e2af2b3
Merge branch 'dev' into fix/mark2
builderjer Aug 17, 2025
4de31b1
Re-added Mark 1 stuff
builderjer Aug 17, 2025
5e6aec1
Re-added hifi-berry device
builderjer Aug 17, 2025
3341703
readded mk1 and hifiberry
builderjer Aug 17, 2025
3f76107
removed commented lines
builderjer Aug 17, 2025
f94bc48
removed duplicate function
builderjer Aug 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 35 additions & 26 deletions ovos-i2csound
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,48 +62,33 @@ get_board_version() {
done
}

# Better version comparison
sort_version() {
[ "$(printf '%s\n' "$1" "$2" | sort -V | head -n1)" != "$1" ]
}

# Checks for kernal compat
compare_kernel_version() {
current_kernel=$(uname -r | cut -d '-' -f1)
if sort_version "$current_kernel" "$1"; then
return 0
else
return 1
fi
}

# Check for a Mark 1 device
check_mark_1() {
local timeout=3
local dev="/dev/ttyAMA0"

# Check if device exists and is accessible
if [[ ! -c "$dev" ]] || [[ ! -r "$dev" ]] || [[ ! -w "$dev" ]]; then
echo "Error: $dev not accessible" >&2
return 1
fi

# Use file descriptor to ensure proper cleanup
exec 3<>"$dev"

# Clear any pending input
read -t 1 -n 1000 <&3 || true

# Send command
echo "system.version" >&3

# Read response with timeout
read -t "$timeout" -n 128 RESP <&3
local status=$?

# Cleanup
exec 3>&-

Comment on lines 89 to +91

@coderabbitai coderabbitai Bot Aug 17, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Close both ends of FD 3 to avoid a persistent TTY handle leak

exec 3<>"$dev" opens FD 3 for read+write, but only the write end is closed. This leaks the read end and can hold the serial device open past function scope.

Apply this diff:

-    exec 3>&-
+    # Close both read and write ends of FD 3
+    exec 3>&- 3<&-
📝 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.

Suggested change
# Cleanup
exec 3>&-
# Cleanup
# Close both read and write ends of FD 3
exec 3>&- 3<&-
🤖 Prompt for AI Agents 
In ovos-i2csound around lines 89 to 91, the cleanup currently only closes FD 3's
write end which leaks the read end and can keep the TTY open; after using exec
3<>"$dev" explicitly close both ends of FD 3 (close the read and write
descriptors) — e.g. perform both the write-close and read-close (exec 3>&- and
exec 3<&- or a combined close) in the cleanup section so the serial device is
fully released.

# Check response
if [[ $status -eq 0 ]] && [[ "$RESP" == *"Command"* ]]; then
echo "true"
Expand All @@ -118,6 +103,9 @@ main() {
local i2c_device_name
local i2c_device_file="/etc/OpenVoiceOS/i2c_platform"

# Variable to hold current kernel
local current_kernel=$(uname -r | sed -re 's/(^[0-9]*\.[0-9]*\.[0-9]*-[0-9]*)(.*)/\1/')

# Check for an existing file and remove it if there
if [[ -f $i2c_device_file ]]; then
rm -f $i2c_device_file
Expand All @@ -141,6 +129,11 @@ main() {
echo "WM8XXX based HAT found"
mk1=$(check_mark_1)
if [[ "$mk1" == "true" ]]; then
<<<<<<< HEAD
# atmega328p=$(avrdude -p atmega328p -C /etc/avrdude-gpio.conf -c linuxgpio -U signature:r:-:i -F 2>/dev/null | head -n1)
# if [ "${atmega328p}" == ":030000001E950F3B" ] ; then
=======
>>>>>>> e2af2b36e849b943e116f26380ef4f81b43737f8
detection_results[MARK1]=true
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
echo "Mark-1 enclosure $MARK1"
else
Expand Down Expand Up @@ -225,7 +218,7 @@ main() {
amixer -c "seeed4micvoicec" cset numid=8 13
fi
i2c_device_name="RESPEAKER4"
fi
fi

if [[ ${detection_results[RESPEAKER6]} == true ]] && [[ ${detection_results[RESPEAKER4]} == true ]] ; then
echo "Installing and configuring ReSpeaker 6mic"
Comment on lines 218 to 219

@coderabbitai coderabbitai Bot Aug 17, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

6-mic ReSpeaker condition is unreachable; fix the logic.

Requiring both RESPEAKER6 and RESPEAKER4 to be true makes this block never execute. Likely intended: run when 6-mic is present and 4-mic is not.

Apply:

-    if [[ ${detection_results[RESPEAKER6]} == true ]] && [[ ${detection_results[RESPEAKER4]} == true ]] ; then
+    if [[ ${detection_results[RESPEAKER6]} == true ]] && [[ ${detection_results[RESPEAKER4]} == false ]] ; then
🤖 Prompt for AI Agents 
In ovos-i2csound around lines 223-224, the conditional requires both RESPEAKER6
and RESPEAKER4 to be true making the 6-mic branch unreachable; change the logic
to run when RESPEAKER6 is true and RESPEAKER4 is not true (or simply when
RESPEAKER6 is true and RESPEAKER4 == false) so the script installs/configures
ReSpeaker 6mic only if the 6-mic is detected and the 4-mic is not present.

Expand Down Expand Up @@ -257,12 +250,15 @@ fi
i2c_device_name="ADAFRUIT"
fi

# This is called when a Mycroft Mark 2 device is detected
# It will only work if the VocalFusion drivers are correctly installed
# For more information see https://github.com/OpenVoiceOS/VocalFusionDriver
if [[ ${detection_results[TAS5806]} == true ]] ; then
echo "Installing and configuring SJ-201 HAT"
# Initializing XMOS xvf3510

# Check for kernel version
if $(compare_kernel_version "6.6"); then
if [[ $current_kernel =~ "6.6." ]]; then
dtoverlay xvf3510
else
if [[ "$board_result" == "RPI5" ]]; then
Expand All @@ -272,7 +268,7 @@ fi
fi
fi

/usr/libexec/xvf3510-flash --direct "/usr/lib/firmware/xvf3510/app_xvf3510_int_spi_boot_v4_1_0.bin"
/usr/libexec/xvf3510-flash --direct "/usr/lib/firmware/xvf3510/app_xvf3510_int_spi_boot_v4_2_0.bin"
# Initializing Texas Instruments 5806 Amplifier
/usr/bin/tas5806-init
if [[ ${detection_results[SJ201LED]} == true ]] ; then
Comment on lines +266 to 269

@coderabbitai coderabbitai Bot Aug 17, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Guard firmware flashing with existence checks and actionable errors.

Hard fails here can brick the flow. Verify binary and firmware are present/executable/readable and surface a clear error.

Apply this diff:

-        /usr/libexec/xvf3510-flash --direct "/usr/lib/firmware/xvf3510/app_xvf3510_int_spi_boot_v4_2_0.bin"
+        if [[ -x /usr/libexec/xvf3510-flash && -r /usr/lib/firmware/xvf3510/app_xvf3510_int_spi_boot_v4_2_0.bin ]]; then
+            /usr/libexec/xvf3510-flash --direct "/usr/lib/firmware/xvf3510/app_xvf3510_int_spi_boot_v4_2_0.bin"
+        else
+            echo "Error: Missing xvf3510 flasher or firmware image; cannot initialize XMOS" >&2
+        fi

If you want, I can add retries/backoff or a pre-check that the xvf3510 device node is present before flashing.

🏁 Script executed:

#!/bin/bash
set -eux

# Locate the flashing call and its surrounding context
rg -nP "/usr/libexec/xvf3510-flash" -C3

# Check for existing guards around the amplifier initializer
rg -nP "tas5806-init" -C3

Length of output: 969

Add safety checks before flashing firmware

The xvf3510-flash call at ovos-i2csound:231 runs without verifying that the flasher or firmware image is present, which can hard-fail and disrupt startup. Please wrap it in an existence check with a clear error path:

• File: ovos-i2csound
• Lines: 231–234

Suggested diff:

-        /usr/libexec/xvf3510-flash --direct "/usr/lib/firmware/xvf3510/app_xvf3510_int_spi_boot_v4_2_0.bin"
+        if [[ -x /usr/libexec/xvf3510-flash && -r /usr/lib/firmware/xvf3510/app_xvf3510_int_spi_boot_v4_2_0.bin ]]; then
+            /usr/libexec/xvf3510-flash --direct "/usr/lib/firmware/xvf3510/app_xvf3510_int_spi_boot_v4_2_0.bin"
+        else
+            echo "Error: xvf3510 flasher or firmware image missing/unreadable; cannot initialize XMOS" >&2
+            exit 1
+        fi

This ensures any missing executable or firmware is caught early with an actionable message.

Expand All @@ -292,7 +288,7 @@ fi
i2c_device_name="SJ201V10"
fi
echo "Configuring buttons"
dtoverlay sj201-buttons-overlay

if [[ "$board_result" == "RPI5" ]]; then
dtoverlay sj201-buttons-overlay-pi5
else
Expand Down Expand Up @@ -354,13 +350,26 @@ fi
fi
fi

if [[ ${detection_results[HIFIBERRYPRO]} == true ]] ; then
echo "Installing HiFi Berry Pro"
dtoverlay hifiberry-dacplus-pro
i2c_device_name="HIFIBERRYPRO"

if [[ ${detection_results[HIFIBERRYAMP]} == true ]] ; then
echo "Enabling hifi berry headphone jack"
/usr/sbin/i2cset -y 1 0x60 0x01 0xc0
/usr/sbin/i2cset -y 1 0x60 0x02 0x31
fi
fi

# Write the detection results to a file only if something is detected
if [[ -n "$i2c_device_name" ]]; then
if [[ -d "$(dirname "$i2c_device_file")" ]]; then
echo "$i2c_device_name" > "$i2c_device_file" || echo "Error: Failed to write to $i2c_device_file" >&2
else
echo "Error: Directory $(dirname "$i2c_device_file") does not exist" >&2
fi
fi
}

# Run the main function
Expand Down