Added bikkura pon#4
Conversation
Summary of ChangesHello @samitani, 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 introduces a new embedded project named "BikkuraPon" designed for the M5Stamp C3U microcontroller. The project integrates various hardware components like an OLED display, a servo motor, a CDS light sensor, and a buzzer to create an interactive lottery system. Users can interact with the device by triggering the light sensor, which counts "plates" and initiates a lottery sequence with visual and auditory feedback upon reaching a predefined count. 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
|
6ad6f91 to
4cda020
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces the 'BikkuraPon' project, a lottery game for the M5Stamp C3U. The implementation is a solid first step, but there are several opportunities for improvement. My review focuses on enhancing maintainability and responsiveness. I've identified several typos in both the documentation and the code, suggested improvements to naming conventions and comment consistency, and pointed out the use of magic numbers. A key recommendation is to replace blocking delay() calls with a non-blocking approach using millis() to improve the device's responsiveness. Additionally, providing more informative feedback upon hardware initialization failure would improve the user experience.
| PlateCount++; | ||
| isBlocked = true; | ||
| updateDisplay(cdsValue); | ||
| delay(200); // チャタリング防止 |
There was a problem hiding this comment.
Using delay(200) for debouncing blocks the entire loop() function. This makes the device unresponsive to other inputs (like the button) during the delay. For a more responsive system, it's highly recommended to use a non-blocking approach with millis(). You can achieve a non-blocking cooldown by recording the time of detection with millis() and only allowing the state to change after a certain interval has passed.
| # What | ||
|
|
||
|
|
| |-----|------------------|----------------------| | ||
| | U1 | Micro Controller | M5Stamp C3U x 1 | | ||
| | U2 | OLED Display | [0.96インチ 128×64ドット有機ELディスプレイ](https://akizukidenshi.com/catalog/g/g112031/) x 1 | | ||
| | M1 | Servo Motor | MG996R (360 digree rotation)| |
| | M1 | Servo Motor | MG996R (360 digree rotation)| | ||
| | C1 | Capacitor | 470uF x 1 | | ||
| | R1 | Sensor | [Cdsセル 光センサー](https://akizukidenshi.com/catalog/g/g100110/)| | ||
| | R2 | Registance | Carbon Registance (2K ohm) | |
| #define ROTTERY_PLATE_COUNT 5 | ||
| #define ROTTERY_PROBABILITY 2 |
There was a problem hiding this comment.
There's a recurring typo: "ROTTERY" should be "LOTTERY". This should be corrected here and wherever these macros are used for better readability and maintainability.
| #define ROTTERY_PLATE_COUNT 5 | |
| #define ROTTERY_PROBABILITY 2 | |
| #define LOTTERY_PLATE_COUNT 5 | |
| #define LOTTERY_PROBABILITY 2 |
| #define ROTTERY_PLATE_COUNT 5 | ||
| #define ROTTERY_PROBABILITY 2 | ||
|
|
||
| bool isBlocked = false; // 遮蔽フラグ |
There was a problem hiding this comment.
This comment is in Japanese, while other parts of the code use English comments. To ensure consistency and make the code more accessible, it's best to use a single language for comments, preferably English.
| bool isBlocked = false; // 遮蔽フラグ | |
| bool isBlocked = false; // Flag to indicate if the light sensor is blocked |
| #define ROTTERY_PROBABILITY 2 | ||
|
|
||
| bool isBlocked = false; // 遮蔽フラグ | ||
| int PlateCount = 0; |
| if(!display.begin(SSD1306_SWITCHCAPVCC, 0x3C)) { | ||
| for(;;); | ||
| } |
There was a problem hiding this comment.
Halting with an infinite loop (for(;;);) on initialization failure is effective, but it provides no feedback on what went wrong. It would be more user-friendly to print an error message to the Serial monitor before halting. Also, while(true); is often considered more readable than for(;;); for an infinite loop.
| if(!display.begin(SSD1306_SWITCHCAPVCC, 0x3C)) { | |
| for(;;); | |
| } | |
| if(!display.begin(SSD1306_SWITCHCAPVCC, 0x3C)) { | |
| Serial.println("Failed to initialize SSD1306 display."); | |
| while(true); | |
| } |
| updateDisplay(cdsValue); | ||
| delay(200); // チャタリング防止 | ||
| } | ||
| else if (cdsValue > threshold + 200) { |
bf140f9 to
14ea29d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds the 'BikkuraPon' project. The initial code is a good foundation, but it has a critical flaw in its reliance on blocking delay() calls, which makes the device unresponsive. I've provided a high-severity comment explaining this issue and recommending a refactor to a non-blocking state machine. Additionally, I've included several medium-severity suggestions to improve project documentation, dependency management for reproducible builds, and code maintainability. There is also a critical suggestion to improve debuggability in case of hardware initialization failure.
| if(!display.begin(SSD1306_SWITCHCAPVCC, 0x3C)) { | ||
| for(;;); | ||
| } |
There was a problem hiding this comment.
The code enters an infinite loop for(;;); if the display fails to initialize. This will cause the device to hang without any indication of what went wrong, making it very difficult to debug. It's better to provide some feedback to the user, for example, by printing an error message to the Serial monitor.
| if(!display.begin(SSD1306_SWITCHCAPVCC, 0x3C)) { | |
| for(;;); | |
| } | |
| if(!display.begin(SSD1306_SWITCHCAPVCC, 0x3C)) { | |
| Serial.println(F("SSD1306 allocation failed")); | |
| for(;;); | |
| } |
| void loop() { | ||
| int buttonValue = digitalRead(M5LED_BUTTON); | ||
|
|
||
| if (!buttonValue) { | ||
| mg996r360.write(SERVO_CLOCKWISE_SLOW); | ||
| delay(200); | ||
| } else { | ||
| mg996r360.write(SERVO_STOP); | ||
| } | ||
|
|
||
| int cdsValue = analogRead(CDS_PIN); | ||
|
|
||
| // Serial.print("Sensor Value: "); | ||
| // Serial.println(lightValue); | ||
|
|
||
| // 光が遮られた(値が小さくなった)時の判定 | ||
| updateDisplay(cdsValue); | ||
|
|
||
| if (cdsValue < threshold && !isBlocked) { | ||
| plateCount++; | ||
| isBlocked = true; | ||
| updateDisplay(cdsValue); | ||
| delay(200); // チャタリング防止 | ||
| } | ||
| else if (cdsValue > threshold + 200) { | ||
| isBlocked = false; | ||
| } | ||
|
|
||
| // LOTTERY_PLATE_COUNT 回溜まったら抽選開始 | ||
| if (plateCount >= LOTTERY_PLATE_COUNT) { | ||
| runLottery(); | ||
| plateCount = 0; | ||
| updateDisplay(cdsValue); | ||
| } | ||
| } |
There was a problem hiding this comment.
The code makes extensive use of delay() in the loop() function and in functions called from it (like runLottery() and playWinSound()). This is a blocking operation that halts the microcontroller, preventing it from processing any other tasks like reading sensors or checking for button presses. This can lead to missed inputs and makes the device unresponsive. For example:
delay(200)in the button handling logic (line 155) will make the device sluggish if the button is held.delay(200)for sensor debouncing (line 172) will block other inputs.- The long delays in
runLottery()(up to 3 seconds) will make the device completely unresponsive during the lottery animation.
It is highly recommended to refactor the code to use a non-blocking approach. This is typically done by creating a state machine and using the millis() function to manage time-based events. This will ensure your loop() function executes very quickly and the device remains responsive at all times.
| # What | ||
|
|
There was a problem hiding this comment.
| adafruit/Adafruit GFX Library | ||
| adafruit/Adafruit SSD1306 | ||
| madhephaestus/ESP32Servo |
There was a problem hiding this comment.
For reproducible builds, it's a good practice to pin the versions of all dependencies. Some dependencies have versions specified (Adafruit NeoPixel @ ^1.12.3), but others don't. This can lead to build failures in the future if a library releases a breaking change. Please specify versions for all libraries under lib_deps.
adafruit/Adafruit GFX Library @ ^1.11.9
adafruit/Adafruit SSD1306 @ ^2.5.10
madhephaestus/ESP32Servo @ ^3.0.2
| #define SERVO_CLOCKWISE_SLOW 80 | ||
| #define SERVO_CLOCKWISE_FAST 40 | ||
| #define SERVO_STOP 90 | ||
| #define SERVO_ONELAP_DELAY 3145 |
There was a problem hiding this comment.
The value 3145 for SERVO_ONELAP_DELAY appears to be a 'magic number'. While it's defined as a constant, the value itself isn't self-explanatory. To improve maintainability, please add a comment explaining how this value was determined (e.g., through experimentation or calculation).
| #define SERVO_ONELAP_DELAY 3145 | |
| #define SERVO_ONELAP_DELAY 3145 // Time in ms for one full rotation at SERVO_CLOCKWISE_FAST speed |
| bool isBlocked = false; // 遮蔽フラグ | ||
| int plateCount = 0; | ||
| const int threshold = 400; // 遮蔽判定のしきい値(環境に合わせて調整) |
There was a problem hiding this comment.
The comments in the code are a mix of English and Japanese (e.g., // 遮蔽フラグ). For better maintainability and to make the code accessible to a wider audience, it's recommended to use a single language for all comments, preferably English.
| bool isBlocked = false; // 遮蔽フラグ | |
| int plateCount = 0; | |
| const int threshold = 400; // 遮蔽判定のしきい値(環境に合わせて調整) | |
| bool isBlocked = false; // Plate detection flag | |
| int plateCount = 0; | |
| const int threshold = 400; // Threshold for plate detection (adjust for your environment) |
75d472d to
05529af
Compare
ad99b6a to
b0ef3dd
Compare
ビッくらポン!風のおもちゃ