First PR - #1
Conversation
Co-authored-by: Raphael Dixon <raphaeldixon@google.com> Co-authored-by: Jay Chang <jaydc393@gmail.com>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request introduces Glanceboard, an e-ink display daily planner system powered by Gemini, containing a local FastAPI server, a web dashboard, and a Raspberry Pi updater client. While the implementation is comprehensive, several critical issues must be addressed: a syntax error in firebase.json prevents deployment, and insecure rules in storage.rules and firestore.rules allow unauthorized display overwrites and spoofed device registrations. Additionally, the Raspberry Pi Python script requires better resource management for I2C connections, safer image loading, and proper signal handling. Finally, the frontend JavaScript should avoid deprecated global objects and handle potential NaN values, while the setup scripts need adjustments for non-standard home directories and missing dependencies like unzip.
| "runtime": "python311" | ||
| } | ||
| } |
| // Authenticated users can write (the workshop app uploads the generated image). | ||
| match /devices/{deviceId}/display/{allPaths=**} { | ||
| allow read: if true; | ||
| allow write: if request.auth != null; |
There was a problem hiding this comment.
The current rule allows any authenticated user to write to any device's display folder. This is a security vulnerability that allows a malicious user to overwrite the display image of other users' devices. You should restrict write access to only the owner of the device by performing a Firestore document lookup to verify the owner_uid.
| try: | ||
| bus = smbus2.SMBus(1) | ||
| addr = 0x43 | ||
|
|
||
| # Calibrate: 16V range, 0.01 ohm shunt | ||
| cal = 26868 | ||
| bus.write_i2c_block_data(addr, 0x05, [(cal >> 8) & 0xFF, cal & 0xFF]) | ||
| config = (0x00 << 13) | (0x01 << 11) | (0x0D << 7) | (0x0D << 3) | 0x07 | ||
| bus.write_i2c_block_data(addr, 0x00, [(config >> 8) & 0xFF, config & 0xFF]) | ||
|
|
||
| import time as _t | ||
| _t.sleep(0.1) | ||
|
|
||
| # Re-write calibration (needed for current reading) | ||
| bus.write_i2c_block_data(addr, 0x05, [(cal >> 8) & 0xFF, cal & 0xFF]) | ||
|
|
||
| # Read bus voltage (register 0x02) | ||
| data = bus.read_i2c_block_data(addr, 0x02, 2) | ||
| raw = (data[0] << 8 | data[1]) >> 3 | ||
| voltage = raw * 0.004 | ||
|
|
||
| # Read current (register 0x04) — signed 16-bit | ||
| data = bus.read_i2c_block_data(addr, 0x04, 2) | ||
| raw_current = data[0] << 8 | data[1] | ||
| if raw_current > 32767: | ||
| raw_current -= 65536 # Convert to signed | ||
| current_ma = raw_current * 0.1 # Scale depends on calibration | ||
|
|
||
| # Percentage: 3.0V = 0%, 4.2V = 100% | ||
| pct = (voltage - 3.0) / 1.2 * 100 | ||
| pct = max(0.0, min(100.0, pct)) | ||
|
|
||
| bus.close() | ||
| return voltage, pct, current_ma |
There was a problem hiding this comment.
| image = Image.open(io.BytesIO(image_data)) | ||
| image = image.resize((DISPLAY_WIDTH, DISPLAY_HEIGHT), Image.LANCZOS) | ||
|
|
||
| # Overlay battery indicator only when critically low | ||
| voltage, pct, _ = read_battery() | ||
| if pct is not None: | ||
| log.info(f"Battery: {pct:.0f}% ({voltage:.2f}V)") | ||
| if pct < 5: | ||
| image = draw_battery_indicator(image, pct) | ||
| else: | ||
| log.info("Battery: unavailable") | ||
|
|
||
| if not DRIVER_AVAILABLE: | ||
| log.info( | ||
| f"[DRY RUN] Would display image " | ||
| f"({image.size[0]}×{image.size[1]})" | ||
| ) | ||
| return True | ||
|
|
||
| epd = None | ||
| try: | ||
| epd = epd7in3e.EPD() | ||
| epd.init() | ||
| log.info("Display initialized, pushing image...") | ||
|
|
||
| epd.display(epd.getbuffer(image)) | ||
| log.info("Image rendered on display") | ||
|
|
||
| epd.sleep() | ||
| log.info("Display entered sleep mode") | ||
| return True | ||
|
|
||
| except Exception as e: | ||
| log.error(f"Display error: {e}") | ||
| if epd: | ||
| try: | ||
| epd.sleep() | ||
| except Exception: | ||
| pass | ||
| return False |
There was a problem hiding this comment.
Image loading and resizing are performed outside the try...except block. If the downloaded image data is corrupted or incomplete, Image.open will raise an exception (such as UnidentifiedImageError), causing the script to crash. Wrapping the entire function body in the try...except block ensures robust error handling.
| # On battery but in grace period — wait then re-check | ||
| log.info(f"Grace period: waiting {remaining}s then will re-evaluate...") | ||
| _sleep(remaining) | ||
| run_once(halt=halt) # Re-run after grace period expires |
There was a problem hiding this comment.
If the script receives a termination signal (such as SIGINT or SIGTERM) during the grace period sleep, _sleep will return early and _running will be set to False. However, the script will still recursively call run_once and attempt to fetch and display the image instead of exiting gracefully. You should check _running before re-running run_once.
| const d = deviceDoc.exists() ? deviceDoc.data() : c; // fallback to config | ||
|
|
||
| // User-level fields | ||
| const apiKey = c.openrouter_api_key || ""; |
There was a problem hiding this comment.
The code references the deprecated and non-standard global event object, which can cause a ReferenceError in browsers like Firefox. Since openCharacterModal is always called with an object containing the type property (e.g., { type: "kid" } or { type: "extra" }) when creating a new character, existing is never falsy, making the event check dead code. This can be safely simplified.
| const maxEmails = parseInt($("#cfg-email-max")?.value || "5", 10); | ||
| widgetConfigs.email = { ...(widgetConfigs.email || {}), max_emails: Math.max(1, Math.min(10, maxEmails || 5)) }; |
There was a problem hiding this comment.
If #cfg-email-max is not present in the DOM (e.g., when Gmail is not authorized) or contains an invalid non-numeric value, parseInt can result in NaN. Passing NaN to Math.min and Math.max will result in NaN, which gets saved into widgetConfigs.email.max_emails. It is safer to validate the parsed integer with isNaN and fall back to a default value.
| // Authenticated users can read any device and write their own. | ||
| match /devices/{devId} { | ||
| allow read: if request.auth != null; | ||
| allow create: if request.auth != null; |
There was a problem hiding this comment.
| WorkingDirectory=/home/$CURRENT_USER/glanceboard | ||
| EnvironmentFile=/home/$CURRENT_USER/.glanceboard.conf | ||
| ExecStart=/usr/bin/python3 /home/$CURRENT_USER/glanceboard/display_update.py --once |
There was a problem hiding this comment.
Hardcoding /home/$CURRENT_USER assumes that the user's home directory is always located under /home. This will fail if the script is run by a user with a non-standard home directory (such as root whose home is /root, or users on custom system configurations). Using systemd's %h specifier dynamically resolves to the home directory of the user specified in User=, making the service file much more robust.
| sudo apt-get install -y \ | ||
| python3-pip \ | ||
| python3-pil \ | ||
| python3-numpy \ | ||
| python3-spidev \ | ||
| python3-gpiozero \ | ||
| git |
There was a problem hiding this comment.
Added copyright and licensing information for software and materials.
No description provided.