Einstein: port to FreeBSD#208
Conversation
Port Einstein to FreeBSD While there, handle NetBSD Don't break existing Linux and OpenBSD
📝 WalkthroughWalkthroughThe PR broadens BSD platform detection from OpenBSD-specific checks to all BSD variants across CMake build paths, including dependency discovery and source selection. It also changes PulseAudio playback to buffered callback-driven output, synchronizes output state, adjusts GNU compiler warnings, and adds ChangesPlatform portability and audio output
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Emulator
participant TPulseAudioSoundManager
participant TCircleBuffer
participant PulseAudio
Emulator->>TPulseAudioSoundManager: ScheduleOutput(audio data)
TPulseAudioSoundManager->>TCircleBuffer: buffer audio bytes
PulseAudio->>TPulseAudioSoundManager: PAStreamWriteCallback(requested bytes)
TPulseAudioSoundManager->>TCircleBuffer: consume queued bytes
TPulseAudioSoundManager->>PulseAudio: pa_stream_write(buffered bytes)
PulseAudio->>TPulseAudioSoundManager: underflow notification
TPulseAudioSoundManager->>TPulseAudioSoundManager: RequestOutputInterrupt()
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CMakeLists.txt (1)
693-693: Consider adding a broader clang-format search.Adding
clang-format19is good, but the naming convention varies across systems. FreeBSD ports typically install asclang-format<version>without a hyphen (e.g.,clang-format19), while some systems useclang-format-19.Optional: Expand search names for broader compatibility
find_program(CLANG_FORMAT_EXECUTABLE - NAMES clang-format-14 clang-format-mp-14 clang-format19 clang-format + NAMES clang-format-14 clang-format-mp-14 clang-format19 clang-format-19 clang-format HINTS /usr/local/opt/clang-format@14/bin/ /usr/lib/llvm-14/bin/ DOC "clang-format executable")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` at line 693, Update the NAMES list used when locating clang-format in CMakeLists.txt to include additional common variants so the find command matches both hyphenated and non-hyphenated installs; specifically add names like clang-format-19, clang-format-14, clang-format-15, clang-format-16 (and their non-hyphenated counterparts) alongside the existing clang-format and clang-format19 entries in the NAMES list so the finder used in this CMakeLists.txt will locate clang-format across more platforms.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CMakeLists.txt`:
- Line 693: Update the NAMES list used when locating clang-format in
CMakeLists.txt to include additional common variants so the find command matches
both hyphenated and non-hyphenated installs; specifically add names like
clang-format-19, clang-format-14, clang-format-15, clang-format-16 (and their
non-hyphenated counterparts) alongside the existing clang-format and
clang-format19 entries in the NAMES list so the finder used in this
CMakeLists.txt will locate clang-format across more platforms.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CMakeLists.txtEmulator/NativeCalls/CMakeLists.txtEmulator/Serial/CMakeLists.txtEmulator/Sound/CMakeLists.txt
|
Looking at the "nitpick" comment from coderabbit, I realize that the addition of |
|
Looks good to me, @MatthiasWM any thoughts? |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| target_compile_options(EinsteinFLGUI PUBLIC -Wno-stringop-truncation) | ||
| endif() | ||
| target_compile_definitions ( Einstein PRIVATE | ||
| TARGET_UI_FLTK=1 TARGET_OS_LINUX=1 |
There was a problem hiding this comment.
The widened .*BSD$ branch still defines BSD builds as TARGET_OS_LINUX=1, so FreeBSD, NetBSD, and OpenBSD skip the existing BSD compile guards and enter Linux-specific paths. A BSD build can then miss BSD-only headers or ioctl handling, such as code guarded as !TARGET_OS_LINUX, and fail to compile or run networking through the wrong platform path.
|
|
||
| find_program(CLANG_FORMAT_EXECUTABLE | ||
| NAMES clang-format-14 clang-format-mp-14 clang-format | ||
| NAMES clang-format-14 clang-format-mp-14 clang-format19 clang-format |
There was a problem hiding this comment.
This now prefers clang-format19 over an unversioned clang-format when clang-format-14 is absent. On systems where local formatting uses this CMake target but other hooks or CI still use clang-format 13 or 14, the same source tree can be reformatted differently and make formatting checks fail.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if (CMAKE_C_COMPILER_ID STREQUAL "GNU" OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU") | ||
| target_compile_options(Einstein PUBLIC -Wno-stringop-truncation) | ||
| target_compile_options(EinsteinTests PUBLIC -Wno-stringop-truncation) | ||
| target_compile_options(EinsteinFLGUI PUBLIC -Wno-stringop-truncation) | ||
| endif() |
There was a problem hiding this comment.
-Wno-stringop-truncation to the whole target when either compiler is GNU. In a mixed setup such as CC=gcc CXX=clang++, the condition is true, so the C++ compile for EinsteinTests and EinsteinFLGUI can still receive this GCC-only option while building with -Werror. That can fail the build even though the C++ compiler is not GNU. Apply the option per language and compiler instead of at target scope.
| target_compile_options(EinsteinFLGUI PUBLIC -Wno-stringop-truncation) | ||
| endif() | ||
| target_compile_definitions ( Einstein PRIVATE | ||
| TARGET_UI_FLTK=1 TARGET_OS_LINUX=1 |
There was a problem hiding this comment.
TARGET_OS_LINUX=1. That sends BSD compiles through Linux-only preprocessor paths; for example the platform definitions select the Linux endian header path instead of the BSD <sys/endian.h> path. A FreeBSD or NetBSD build can therefore fail before the new BSD source selection helps. Define a BSD platform macro for .*BSD$ targets instead of reusing the Linux one.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Emulator/Sound/TPulseAudioSoundManager.cpp`:
- Around line 59-63: Initialize every PulseAudio pointer member, including
mPAContext and related mainloop/stream pointers, to nullptr in the
TPCircleBufferSoundManager constructor initializer list before any setup calls.
This ensures the error path can safely evaluate pointer checks when
pa_threaded_mainloop_new() or pa_threaded_mainloop_get_api() fails.
- Around line 232-262: Update the teardown logic in the destructor to call
pa_stream_unref(mOutputStream) after pa_stream_disconnect and
pa_context_unref(mPAContext) after pa_context_disconnect, guarding each with its
existing null check and preserving the mainloop locking order.
- Line 497: Replace the non-portable MIN call in the relevant sound manager
method with std::min<size_t>(mOutputBuffer->AvailableBytes(), bytesToWrite), and
add the <algorithm> include needed for std::min.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05ac1a76-c105-47df-8ebf-da9d1d1f76d2
📒 Files selected for processing (2)
Emulator/Sound/TPulseAudioSoundManager.cppEmulator/Sound/TPulseAudioSoundManager.h
| mOutputBuffer(new TCircleBuffer( | ||
| kNewtonBufferSizeInFrames * 4 * sizeof(KUInt16))), | ||
| mDataMutex(new TMutex()), | ||
| mOutputIsRunning(false) | ||
| mOutputIsRunning(false), | ||
| mOutputRequestPending(false) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant files
git ls-files | rg 'Emulator/Sound/TPulseAudioSoundManager\.(cpp|h|hpp)$'
# Show constructor and error-path sections with line numbers
for f in Emulator/Sound/TPulseAudioSoundManager.cpp Emulator/Sound/TPulseAudioSoundManager.h; do
if [ -f "$f" ]; then
echo "===== $f ====="
nl -ba "$f" | sed -n '1,260p'
fi
doneRepository: pguyot/Einstein
Length of output: 333
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in Emulator/Sound/TPulseAudioSoundManager.cpp Emulator/Sound/TPulseAudioSoundManager.h; do
if [ -f "$f" ]; then
echo "===== $f ====="
# Show file with line numbers using cat -n, focused on likely relevant ranges
case "$f" in
*.cpp) sed -n '1,260p' "$f" | cat -n | sed -n '1,260p' ;;
*.h) sed -n '1,240p' "$f" | cat -n | sed -n '1,240p' ;;
esac
fi
done
# Also extract just the constructor/error label vicinity for quicker review
echo "===== focused constructor/error-path search ====="
rg -n -A4 -B4 'pa_threaded_mainloop_new|goto error|pa_context_errno|mPAContext|mPAMainLoop|mPAMainLoopAPI|mOutputStream|mPAOperation|mPAOperationDescr' Emulator/Sound/TPulseAudioSoundManager.cpp Emulator/Sound/TPulseAudioSoundManager.hRepository: pguyot/Einstein
Length of output: 38255
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in Emulator/Sound/TPulseAudioSoundManager.cpp Emulator/Sound/TPulseAudioSoundManager.h; do
if [ -f "$f" ]; then
echo "===== $f ====="
case "$f" in
*.cpp) sed -n '1,260p' "$f" | cat -n | sed -n '1,260p' ;;
*.h) sed -n '1,240p' "$f" | cat -n | sed -n '1,240p' ;;
esac
fi
done
echo "===== focused constructor/error-path search ====="
rg -n -A4 -B4 'pa_threaded_mainloop_new|goto error|pa_context_errno|mPAContext|mPAMainLoop|mPAMainLoopAPI|mOutputStream|mPAOperation|mPAOperationDescr' Emulator/Sound/TPulseAudioSoundManager.cpp Emulator/Sound/TPulseAudioSoundManager.hRepository: pguyot/Einstein
Length of output: 38255
Initialize the PulseAudio pointer members in the ctor init list
mPAContext is read on the error: path before any assignment if pa_threaded_mainloop_new() or pa_threaded_mainloop_get_api() fails, so the current if (mPAContext) check can hit undefined behavior. Initialize all pointer members to nullptr up front.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Emulator/Sound/TPulseAudioSoundManager.cpp` around lines 59 - 63, Initialize
every PulseAudio pointer member, including mPAContext and related
mainloop/stream pointers, to nullptr in the TPCircleBufferSoundManager
constructor initializer list before any setup calls. This ensures the error path
can safely evaluate pointer checks when pa_threaded_mainloop_new() or
pa_threaded_mainloop_get_api() fails.
| if (mPAMainLoop) | ||
| { | ||
| pa_threaded_mainloop_lock(mPAMainLoop); | ||
| } | ||
| pa_stream_set_write_callback(mOutputStream, NULL, NULL); | ||
| pa_stream_set_underflow_callback(mOutputStream, NULL, NULL); | ||
| pa_stream_set_state_callback(mOutputStream, NULL, NULL); | ||
| // disconnect the stream | ||
| pa_stream_disconnect(mOutputStream); | ||
| pa_context_disconnect(mPAContext); | ||
| // pa_threaded_mainloop_stop | ||
| if (mPAContext) | ||
| { | ||
| pa_context_disconnect(mPAContext); | ||
| } | ||
| if (mPAMainLoop) | ||
| { | ||
| pa_threaded_mainloop_unlock(mPAMainLoop); | ||
| } | ||
| } | ||
| if (mPAMainLoop) | ||
| { | ||
| pa_threaded_mainloop_stop(mPAMainLoop); | ||
| pa_threaded_mainloop_free(mPAMainLoop); | ||
| } | ||
| if (mDataMutex) | ||
| { | ||
| delete mDataMutex; | ||
| } | ||
| if (mOutputBuffer) | ||
| { | ||
| delete mOutputBuffer; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether stream/context are ever unref'd in this manager.
fd -t f 'TPulseAudioSoundManager.cpp' --exec rg -n 'pa_stream_unref|pa_context_unref|pa_stream_disconnect|pa_context_disconnect' {}Repository: pguyot/Einstein
Length of output: 315
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='Emulator/Sound/TPulseAudioSoundManager.cpp'
# Show the constructor error path and destructor around the relevant lines.
sed -n '180,260p' "$file" | cat -n
echo
echo '---'
echo 'All PulseAudio cleanup calls in this file:'
rg -n 'pa_stream_unref|pa_context_unref|pa_stream_disconnect|pa_context_disconnect|pa_threaded_mainloop_stop|pa_threaded_mainloop_free' "$file"Repository: pguyot/Einstein
Length of output: 2885
Unref the PulseAudio stream and context on teardown pa_stream_disconnect()/pa_context_disconnect() only disconnect; this destructor still leaves both objects allocated on the normal path. Add pa_stream_unref(mOutputStream) and pa_context_unref(mPAContext) after disconnecting, mirroring the constructor error path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Emulator/Sound/TPulseAudioSoundManager.cpp` around lines 232 - 262, Update
the teardown logic in the destructor to call pa_stream_unref(mOutputStream)
after pa_stream_disconnect and pa_context_unref(mPAContext) after
pa_context_disconnect, guarding each with its existing null check and preserving
the mainloop locking order.
| } | ||
|
|
||
| mDataMutex->Lock(); | ||
| bytesToWrite = MIN(mOutputBuffer->AvailableBytes(), bytesToWrite); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether MIN is defined anywhere reachable and how it's provided.
rg -nP '\bdefine\s+MIN\b'
rg -nP '`#include`\s*<sys/param\.h>|`#include`\s*<algorithm>' Emulator/Sound/TPulseAudioSoundManager.cppRepository: pguyot/Einstein
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== File outline =="
ast-grep outline Emulator/Sound/TPulseAudioSoundManager.cpp --view expanded || true
echo
echo "== Relevant file sections =="
nl -ba Emulator/Sound/TPulseAudioSoundManager.cpp | sed -n '1,120p'
echo
nl -ba Emulator/Sound/TPulseAudioSoundManager.cpp | sed -n '460,520p'
echo
echo "== Search for MIN and sys/param usage =="
rg -n --hidden --glob '!**/node_modules/**' '\bMIN\b|sys/param\.h|<algorithm>' Emulator/Sound . || trueRepository: pguyot/Einstein
Length of output: 550
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Top of file =="
sed -n '1,120p' Emulator/Sound/TPulseAudioSoundManager.cpp
echo
echo "== Relevant lines =="
sed -n '485,502p' Emulator/Sound/TPulseAudioSoundManager.cpp
echo
echo "== Repo-wide MIN definition search =="
rg -n --hidden --glob '!**/node_modules/**' '^[[:space:]]*#\s*define\s+MIN\b|[\(<[:space:]]MIN[\s\(]' .
echo
echo "== sys/param.h and algorithm includes =="
rg -n --hidden --glob '!**/node_modules/**' '#\s*include\s*<sys/param\.h>|#\s*include\s*<algorithm>' Emulator/Sound .Repository: pguyot/Einstein
Length of output: 6765
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for MIN in K headers =="
rg -n --hidden --glob '!**/node_modules/**' '^[[:space:]]*#\s*define\s+MIN\b|^\s*#\s*include\s*<sys/param\.h>|^\s*#\s*include\s*<algorithm>' K Emulator/Sound app Monitor libffi portaudio || true
echo
echo "== K/Defines/KDefinitions.h =="
sed -n '1,220p' K/Defines/KDefinitions.h
echo
echo "== TPortAudioSoundManager.cpp top =="
sed -n '1,90p' Emulator/Sound/TPortAudioSoundManager.cpp
echo
echo "== TCoreAudioSoundManager.cpp top =="
sed -n '1,70p' Emulator/Sound/TCoreAudioSoundManager.cppRepository: pguyot/Einstein
Length of output: 12937
Use std::min here Emulator/Sound/TPulseAudioSoundManager.cpp:497 doesn’t bring in a portable MIN definition, so BSD builds may depend on a non-transitive system include. Add <algorithm> and use std::min<size_t>(...) instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Emulator/Sound/TPulseAudioSoundManager.cpp` at line 497, Replace the
non-portable MIN call in the relevant sound manager method with
std::min<size_t>(mOutputBuffer->AvailableBytes(), bytesToWrite), and add the
<algorithm> include needed for std::min.
| if (CMAKE_C_COMPILER_ID STREQUAL "GNU" OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU") | ||
| target_compile_options(Einstein PUBLIC -Wno-stringop-truncation) | ||
| target_compile_options(EinsteinTests PUBLIC -Wno-stringop-truncation) | ||
| target_compile_options(EinsteinFLGUI PUBLIC -Wno-stringop-truncation) | ||
| endif() |
There was a problem hiding this comment.
CC=gcc CXX=clang++. These targets compile C++ sources, and target_compile_options() applies the flag to the whole target, so Clang can still receive GCC-only -Wno-stringop-truncation. Because EinsteinTests and EinsteinFLGUI compile with -Werror, that mixed setup can fail the build on an unknown warning option.
| target_compile_options(EinsteinFLGUI PUBLIC -Wno-stringop-truncation) | ||
| endif() | ||
| target_compile_definitions ( Einstein PRIVATE | ||
| TARGET_UI_FLTK=1 TARGET_OS_LINUX=1 |
There was a problem hiding this comment.
TARGET_OS_LINUX=1. That sends BSD builds through Linux preprocessor paths; for example the platform definitions select Linux <endian.h> instead of the BSD <sys/endian.h> branch. A FreeBSD or NetBSD build can fail before the newly selected BSD sources help.
|
Not using any of the BSD variants, just Ubuntu. Is there a specific order to this PR and #220 ? |
No, they should be independent. Thanks for checking them out! PS: Einstein is now part of the official FreeBSD distribution: |
3 participants
Port Einstein to FreeBSD
While there, handle NetBSD
Don't break existing Linux and OpenBSD
Summary by CodeRabbit
Greptile Summary
This PR ports the CMake build paths toward FreeBSD and NetBSD support. The main changes are:
Confidence Score: 4/5
This should be fixed before merging.
CMakeLists.txt
Important Files Changed
Reviews (3): Last reviewed commit: "Einstein: port to FreeBSD" | Re-trigger Greptile