-
Notifications
You must be signed in to change notification settings - Fork 100
Pynest #270
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: develop
Are you sure you want to change the base?
Pynest #270
Conversation
Signed-off-by: Patrick Gehrsitz <[email protected]>
Signed-off-by: Patrick Gehrsitz <[email protected]>
Signed-off-by: Patrick Gehrsitz <[email protected]>
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.
Pull request overview
Copilot reviewed 41 out of 46 changed files in this pull request and generated 25 comments.
Comments suppressed due to low confidence (2)
libs/helper_fn.sh:83
- The function 'run_apt_update' is now duplicated in both 'libs/helper_fn.sh' and 'tools/libs/helper_fn.sh'. This creates code duplication and maintenance issues. Consider consolidating these helper functions into a single location.
[[ "$(is_pi5)" = "0" ]]; then
echo "1"
else
pylibs/v4l2/ctl.py:172
- Call to function get_control_cur_value_with_qc with too many arguments; should be no more than 2.
return get_control_cur_value_with_qc(device_path, qc, control)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| streamer_args += ["--format", "MJPEG", "--encoder", "HW"] | ||
|
|
||
| v4l2ctl = self.parameters["v4l2ctl"] | ||
| if v4l2ctl: |
Copilot
AI
Dec 14, 2025
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.
Variable v4l2ctl is used before being checked for truthiness. If v4l2ctl is an empty string, it will be falsy in the if statement on line 69, but then on line 90 it's split unconditionally. This could result in attempting to split a falsy value that wasn't set. Consider ensuring v4l2ctl is properly initialized or checking it again before line 90.
| if v4l2ctl: | |
| if isinstance(v4l2ctl, str) and v4l2ctl.strip(): |
|
|
||
| reinstall_runtime_env() { | ||
| delete_runtime_env | ||
| install_runtime_env |
Copilot
AI
Dec 14, 2025
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.
Function 'install_runtime_env' is called but is never defined in this file. It should likely be 'setup_runtime_env' based on the function defined on line 153.
| "mainsail-ustreamer; distro_version >= '11'", | ||
| "mainsail-spyglass; vendor == 'raspberry-pi' and distro_version >= '11'", | ||
| "mainsail-camera-streamer-generic; vendor != 'raspberry-pi' and distro_version >= '11'", | ||
| "mainsail-camera-streamer-raspi; vendor == 'raspberry-pi' and distro_version >= '11'", |
Copilot
AI
Dec 14, 2025
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.
Trailing comma on the last item of the JSON array. While this is valid in some JSON parsers, it's not standard JSON and may cause parsing errors in strict JSON parsers. Remove the trailing comma after the last array element.
| def get_control_cur_value_with_qc( | ||
| device_path: str, qc: raw.v4l2_query_ext_ctrl | ||
| ) -> Optional[int]: |
Copilot
AI
Dec 14, 2025
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.
Function signature has incorrect parameter count. The function is defined with two parameters (device_path and qc) but the docstring and the calling code suggest it should only take one parameter (qc). The 'control' parameter mentioned in line 172 is not defined in the function signature.
| loop = asyncio.get_event_loop() | ||
| try: | ||
| loop.run_until_complete(main()) | ||
| finally: | ||
| loop.close() |
Copilot
AI
Dec 14, 2025
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.
Using the deprecated get_event_loop() method. In Python 3.10+, it's recommended to use asyncio.run() instead of manually creating and managing an event loop. This is simpler and handles loop cleanup automatically.
| loop = asyncio.get_event_loop() | |
| try: | |
| loop.run_until_complete(main()) | |
| finally: | |
| loop.close() | |
| asyncio.run(main()) |
|
|
||
|
|
||
| class Libcamera(camera.Camera): | ||
| def __init__(self, path, *args, **kwargs) -> None: |
Copilot
AI
Dec 14, 2025
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.
This class does not call Camera.init during initialization. (Libcamera.init may be missing a call to a base class init)
| def __init__(self, path, *args, **kwargs) -> None: | |
| def __init__(self, path, *args, **kwargs) -> None: | |
| super().__init__(*args, **kwargs) |
|
|
||
|
|
||
| class UVC(camera.Camera): | ||
| def __init__(self, path: str, *args, **kwargs) -> None: |
Copilot
AI
Dec 14, 2025
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.
This class does not call Camera.init during initialization. (UVC.init may be missing a call to a base class init)
| def __init__(self, path: str, *args, **kwargs) -> None: | |
| def __init__(self, path: str, *args, **kwargs) -> None: | |
| super().__init__(path, *args, **kwargs) |
| "max": v.max, | ||
| "default": v.default, | ||
| } | ||
| except ImportError: |
Copilot
AI
Dec 14, 2025
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.
'except' clause does nothing but pass and there is no explanatory comment.
| except ImportError: | |
| except ImportError: | |
| # The libcamera module is not installed; return empty controls. |
| with open("/etc/mainsailos-release", "r") as file: | ||
| content = file.read() | ||
| logger.log_info_silent(f"Release: {content.strip()}", log_pre) | ||
| except FileNotFoundError: |
Copilot
AI
Dec 14, 2025
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.
'except' clause does nothing but pass and there is no explanatory comment.
| except FileNotFoundError: | |
| except FileNotFoundError: | |
| # It's normal for /etc/mainsailos-release to be missing on some systems; ignore if not present. |
| if [[ -z "${CROWSNEST_USTREAMER_REPO_SHIP}" ]]; then | ||
| CROWSNEST_USTREAMER_REPO_SHIP="https://github.com/pikvm/ustreamer.git" | ||
| fi | ||
| if [[ -z "${CROWSNEST_USTREAMER_REPO_BRANCH}" ]]; then | ||
| CROWSNEST_USTREAMER_REPO_BRANCH="master" | ||
| fi | ||
| USTREAMER_PATH="bin/ustreamer" | ||
|
|
||
| # Paths of repos | ||
| ALL_PATHS=( | ||
| "${USTREAMER_PATH}" | ||
| ) | ||
|
|
||
| APPS=("mainsail-ustreamer" "mainsail-spyglass") | ||
| if [[ "$(use_pi_specifics)" = "1" ]]; then | ||
| APPS+=("mainsail-camera-streamer-raspi") | ||
| else | ||
| APPS+=("mainsail-camera-streamer-generic") | ||
| fi | ||
|
|
||
| : "${BASE_USER:=${SUDO_USER:-${USER}}}" | ||
| VENV="/home/${BASE_USER}/crowsnest-env" | ||
|
|
||
| clone_ustreamer() { | ||
| ## remove bin/ustreamer if exist | ||
| if [[ -d bin/ustreamer ]]; then | ||
| rm -rf bin/ustreamer | ||
| fi | ||
| sudo -u "${BASE_USER}" \ | ||
| git clone "${CROWSNEST_USTREAMER_REPO_SHIP}" \ | ||
| -b "${CROWSNEST_USTREAMER_REPO_BRANCH}" \ | ||
| --depth=1 --single-branch "${USTREAMER_PATH}" | ||
| } | ||
|
|
||
| get_avail_mem() { | ||
| grep "MemTotal" /proc/meminfo | awk '{print $2}' | ||
| } | ||
|
|
||
| build_ustreamer() { | ||
| ## Determine Ramsize and export MAKEFLAG | ||
| if [[ "$(get_avail_mem)" -le 524288 ]]; then | ||
| USE_PROCS=-j1 | ||
| elif [[ "$(get_avail_mem)" -le 1048576 ]]; then | ||
| USE_PROCS=-j2 | ||
| else | ||
| USE_PROCS=-j4 | ||
| fi | ||
|
|
||
| if [[ ! -d "${USTREAMER_PATH}" ]]; then | ||
| msg "'${USTREAMER_PATH}' does not exist! Build skipped ... [WARN]\n" | ||
| else | ||
| msg "Build '${USTREAMER_PATH##*/}' using ${USE_PROCS##-j} Cores ... \n" | ||
| pushd "${USTREAMER_PATH}" &> /dev/null || exit 1 | ||
| make "${USE_PROCS}" | ||
| popd &> /dev/null || exit 1 | ||
| msg "Build '${USTREAMER_PATH##*/}' ... [SUCCESS]\n" | ||
| fi |
Copilot
AI
Dec 14, 2025
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.
clone_ustreamer is cloning https://github.com/pikvm/ustreamer.git using the mutable master branch during install/update, then building and running that binary, which introduces a supply-chain risk. If the upstream GitHub repo or the network path is compromised, an attacker can ship malicious commits to master that will be transparently built and executed here (typically as root or with access to video devices). To mitigate this, pin CROWSNEST_USTREAMER_REPO_SHIP/CROWSNEST_USTREAMER_REPO_BRANCH to an immutable tag or commit SHA, or depend on a signed/package-managed ustreamer build instead of cloning a moving branch.
Signed-off-by: Patrick Gehrsitz <[email protected]>
Signed-off-by: Patrick Gehrsitz <[email protected]>
Signed-off-by: Patrick Gehrsitz <[email protected]>
Signed-off-by: Patrick Gehrsitz <[email protected]>
Signed-off-by: Patrick Gehrsitz <[email protected]>
Signed-off-by: Patrick Gehrsitz <[email protected]>
Signed-off-by: Patrick Gehrsitz <[email protected]>
Signed-off-by: Patrick Gehrsitz <[email protected]>
Signed-off-by: Patrick Gehrsitz <[email protected]>
Signed-off-by: Patrick Gehrsitz <[email protected]>
Signed-off-by: Patrick Gehrsitz <[email protected]>
Signed-off-by: Patrick Gehrsitz <[email protected]>
Signed-off-by: Patrick Gehrsitz <[email protected]>
Signed-off-by: Patrick Gehrsitz <[email protected]>
Signed-off-by: Patrick Gehrsitz <[email protected]>
Signed-off-by: Patrick Gehrsitz <[email protected]>
Signed-off-by: Patrick Gehrsitz <[email protected]>
Signed-off-by: Patrick Gehrsitz <[email protected]>
Signed-off-by: Patrick Gehrsitz <[email protected]>
No description provided.