Progress towards getting LED flash working#85
Progress towards getting LED flash working#85acutetech wants to merge 1 commit intorecheck_camera_combinationsfrom
Conversation
Summary of ChangesHello @acutetech, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on advancing the integration of LED flash with motion detection, aiming to ensure proper illumination during both motion detection and subsequent image capture. Key changes involve a significant refactoring of the HM0360 image sensor's preparation and strobe control logic, alongside the introduction of a new utility for precise timing measurements within the image processing and neural network pipeline. The PR also includes general code cleanup, improved variable initialization, and updates to the build system and documentation, addressing feedback from previous reviews. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request focuses on refining the LED flash functionality and motion detection settings for the HM0360 camera. Key changes include refactoring time measurement logic into a utility function, consolidating HM0360 configuration, and updating flash duration. Several configuration files for the HM0360 strobe settings have also been added or modified, reflecting ongoing experimentation. The changes also address minor code cleanliness improvements such as explicit variable initialization and removal of unused variables.
| // CGP - no need to? | ||
| //f_chdir(dirManager->base_dir); |
There was a problem hiding this comment.
The f_chdir(dirManager->base_dir); call is commented out with a note "CGP - no need to?". If this line is truly no longer needed, it should be removed entirely to avoid dead code and potential confusion. If it's a temporary disablement for testing, please add a more descriptive comment explaining the reason and the plan for its eventual removal or re-enablement.
| // CGP - no need to? | ||
| //f_chdir(dirManager->base_dir); |
There was a problem hiding this comment.
Similar to the previous comment, this f_chdir(dirManager->base_dir); call is commented out. Please clarify if it's permanently removed or temporarily disabled, and update the code or comment accordingly.
| uint8_t brightnessPercent; | ||
| uint8_t mdInterval; | ||
|
|
||
| uint16_t mdInterval; |
There was a problem hiding this comment.
The type of mdInterval has been changed from uint8_t to uint16_t. Please add a comment explaining the reason for this type promotion. Is it to support larger motion detection intervals, or for another purpose?
uint16_t mdInterval; // Changed from uint8_t to support larger intervals| // Now merged (above) | ||
| //#ifdef USE_HM0360_MD | ||
| // // HM0360 for motion detection only. | ||
| // // If the camera system is disabled then ensure MD is off and flash LED is off | ||
| // if (hm0360_md_isHM0360Present()) { | ||
| // hm0360_md_prepare(enabled, mdInterval); // select CONTEXT_B registers (if enabled) | ||
| // | ||
| // // Consider turning on the LED flashes, controlled by the HM0360 STROBE output | ||
| // if (!enabled) { | ||
| // // No STROBE pulses | ||
| // xprintf("Camera disabled - no LED flashes\n"); | ||
| // hm0360_md_configureStrobe(0); | ||
| // } | ||
| // else if ((brightnessPercent == 0) || (ledInUse == 0)) { | ||
| // // No STROBE pulses | ||
| // xprintf("Preparing HM0360 for MD - no LED flashes\n"); | ||
| // hm0360_md_configureStrobe(0); | ||
| // } | ||
| // else { | ||
| // // Configure STROBE pulses - NOTE: normal mode is 0x0b = 'dynamic 2' | ||
| // xprintf("Preparing HM0360 for MD - with LED flashes 0x%02x\n", fatfs_getOperationalParameter(OP_PARAMETER_STROBE_MODE)); | ||
| // //hm0360_md_configureStrobe(0x0B); | ||
| // hm0360_md_configureStrobe(fatfs_getOperationalParameter(OP_PARAMETER_STROBE_MODE)); | ||
| // } | ||
| // } | ||
| // else { | ||
| // // HM0360 is missing | ||
| // xprintf("HM0360 missing...\n"); | ||
| // } | ||
| // | ||
| //#endif // USE_HM0360_MD |
There was a problem hiding this comment.
The previous #ifdef USE_HM0360_MD block has been commented out after merging its logic into the combined #if defined(USE_HM0360) || defined(USE_HM0360_MD) block. Once the new consolidated logic is confirmed stable, this large commented-out section should be removed to reduce code clutter and improve readability.
|
|
||
| // Default Duration (ms) that LED Flash is on | ||
| #define FLASHDURATION 100 | ||
| #define FLASHDURATION 50 |
There was a problem hiding this comment.
The FLASHDURATION has been reduced from 100 to 50 milliseconds. Please add a comment explaining the functional reason for this change. For example, is it for power saving, to prevent overexposure, or based on new hardware characteristics?
#define FLASHDURATION 50 // Reduced to 50ms to prevent overexposure1 participant
I am experimenting to find out what is necessary to get the LED flash turning on at the right times so that the motion detection frames are illuminated every 1s (or so) and also the frames which grab the actual images once awake.
This is work in progress and I am not there yet.
However I have detected a problem where the LED turns on and stays on - I have a hypothesis on this and in the spirit of having branches that add just one feature I am going to pause this branch and make a new one "semaphore_for_I2C" to investigate that.
BTW, this branch fixes a couple of points that came up in an earlier review.