Skip to content

Introduce MCP9808 Temperature Sensor Manager #283

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

Merged
merged 10 commits into from
Jul 26, 2025
Merged

Conversation

Mikefly123
Copy link
Member

@Mikefly123 Mikefly123 commented Jul 21, 2025

Summary

This PR adds support for the MCP9808 temperature sensor by implementing a CircuitPython manager that adheres to the TemperatureSensorProto interface. The implementation includes comprehensive unit tests, proper error handling, and follows the established patterns in the codebase.

How was this tested

  • Added new unit tests
  • Ran code on hardware (screenshots are helpful)

Usage Example

from pysquared.hardware.temperature_sensor.manager.mcp9808 import MCP9808Manager
from pysquared.logger import Logger
import busio

logger = Logger()
i2c = busio.I2C(board.SCL, board.SDA)
temp_sensor = MCP9808Manager(logger, i2c, addr=0x18)
temperature = temp_sensor.get_temperature()  # Returns float in Celsius or None

Technical Details

MCP9808Manager Features

Error Handling: Graceful handling of sensor communication failures
Type Safety: Full type annotations with proper forward references
Logging: Integrated debug and error logging

Testing Strategy

  • Mock Classes: Two specialized mock classes for different test scenarios:
  • MockMCP9808WithError: Raises exceptions when temperature is accessed
  • MockMCP9808WithTemperature: Returns configurable temperature values
  • Comprehensive Coverage: Tests all code paths including error conditions
  • Pattern Consistency: Follows the same testing patterns as other sensor managers

@Mikefly123 Mikefly123 self-assigned this Jul 21, 2025
@Mikefly123 Mikefly123 added Vibe Coded This was written mostly by AI with human oversight DRAFT labels Jul 21, 2025
@Mikefly123 Mikefly123 requested a review from Copilot July 21, 2025 00:35
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces support for the MCP9808 temperature sensor by implementing a CircuitPython manager that follows the established TemperatureSensorProto interface. The implementation includes comprehensive error handling, configurable resolution settings, and follows existing codebase patterns.

  • Adds MCP9808Manager class with temperature reading capabilities and configurable resolution
  • Implements comprehensive unit tests with specialized mock classes for different test scenarios
  • Provides proper package structure and mock implementations for testing

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
pysquared/hardware/temperature_sensor/manager/mcp9808.py Core implementation of MCP9808Manager with initialization, error handling, and temperature reading
tests/unit/hardware/temperature_sensor/manager/test_mcp9808_manager.py Comprehensive unit tests covering all code paths including error conditions
mocks/adafruit_mcp9808/mcp9808.py Mock implementation of MCP9808 sensor for testing
pyproject.toml Package configuration updates to include new temperature sensor modules
Various init.py files Package initialization files for proper module structure
Comments suppressed due to low confidence (1)

pysquared/hardware/temperature_sensor/manager/mcp9808.py:65

  • The resolution setting functionality is not covered by any tests. The tests should verify that the resolution parameter is properly set on the MCP9808 instance during initialization.
        self._mcp9808.resolution = resolution

@Mikefly123 Mikefly123 changed the title Introduce MCP9809 Temperature Sensor Manager Introduce MCP9808 Temperature Sensor Manager Jul 21, 2025
@Mikefly123 Mikefly123 marked this pull request as ready for review July 21, 2025 18:21
@Mikefly123 Mikefly123 requested a review from ineskhou July 21, 2025 18:22
@Mikefly123 Mikefly123 removed the DRAFT label Jul 21, 2025
@Mikefly123 Mikefly123 requested a review from nateinaction July 23, 2025 03:30
@nateinaction nateinaction mentioned this pull request Jul 23, 2025
3 tasks
Copy link
Member

@nateinaction nateinaction left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Had some additional feedback around keeping the codebase consistent and reducing test complexity which I put up in #289

self,
logger: Logger,
i2c: I2C,
addr: int = 0x18,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've opted to not include address defaults for other managers. I couldn't find this address mentioned in the datasheet. Is this likely to be the same for all potential implementations of the MCP9808?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, we should not cast a default I2C address here to keep it consistent with the other managers. 0x18 just happens to be the most common address for the MCP9808 on the PROVES Kit, but we can just pass it on init rather than have it be default.

I need to make a docs page sometime soon with a table of device i2c addresses on it.

nateinaction
nateinaction previously approved these changes Jul 25, 2025
Copy link
Member

@nateinaction nateinaction left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LFG!

@Mikefly123
Copy link
Member Author

Hey Nate! Thanks for the review on this I actually think we might have to tweak a thing or two about the PR before merging.

I was trying to get this on hardware (and sorry for not having hardware for you to test this on yet!) and running into two issues:

  1. Init with the TCA causes an unhandled exception.
Adafruit CircuitPython 9.1.0-beta.1-417-g6692d3bed9-dirty on 2025-05-28; PROVESKit FC Board RP2350 V5b with rp2350a
>>> from pysquared.hardware.temperature_sensor.manager.mcp9808 import MCP9808Manager
>>> temp_sensor = MCP9808Manager(logger, tca[1], 27)
{"time": "2000-01-01 00:14:56", "level": "DEBUG", "msg": "Initializing MCP9808 temperature sensor"}
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/lib/pysquared/hardware/temperature_sensor/manager/mcp9808.py", line 68, in __init__
  File "src/flight-software/lib/adafruit_mcp9808.py", line 266, in resolution
  File "src/flight-software/lib/adafruit_register/i2c_bits.py", line 97, in __set__
  File "src/flight-software/lib/adafruit_tca9548a.py", line 70, in unlock
OSError: [Errno 5] Input/output error

This might be an upstream problem with the driver or some weird with the TCA because removing it from the init and trying to do it manually in the REPL causes a stuck bus on the I2C line that is not cleared by pulling mux_reset.

>>> temp_sensor._mcp9808.resolution=1
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "src/flight-software/lib/adafruit_mcp9808.py", line 266, in resolution
  File "src/flight-software/lib/adafruit_register/i2c_bits.py", line 82, in __set__
  File "src/flight-software/lib/adafruit_tca9548a.py", line 63, in try_lock
TimeoutError: Unable to lock I2C bus within timeout period.
  1. Calling get_temperature() just returns the name and memory address rather than the actual value. Possibly a user error on my part?
Adafruit CircuitPython 9.1.0-beta.1-417-g6692d3bed9-dirty on 2025-05-28; PROVESKit FC Board RP2350 V5b with rp2350a
> from pysquared.hardware.temperature_sensor.manager.mcp9808 import MCP9808Manager
> >>> temp_sensor = MCP9808Manager(logger, tca[1], 27)
> {"time": "2000-01-01 00:15:40", "level": "DEBUG", "msg": "Initializing MCP9808 temperature sensor"}
> >>> temp_sensor.get_temperature()
> <Temperature object at 0x2003a480>
> ```

@Mikefly123
Copy link
Member Author

Update on Issue 1, power cycling the faces does seem to clear the stuck I2C bus. Right now this requires manual keyboard interrupt in the REPL, but it may be possible to do this automagically after a watchdog kick

@nateinaction
Copy link
Member

nateinaction commented Jul 26, 2025

Calling get_temperature() just returns the name and memory address rather than the actual value.

That's right. All sensor readings are objects now with timestamps and values. You can use get_temperature().value to get the temp.

@nateinaction
Copy link
Member

sorry for not having hardware for you to test this on yet!

giphy

Copy link

@Mikefly123
Copy link
Member Author

Okay! I have verified that this PR works as intended after removing the resolution setting from the mcp9808 manager!
image

@nateinaction I will be opening #293 to try and tackle the MCP resolution setting bug. For now, if it looks good to you, let's get this thing in!

@Mikefly123 Mikefly123 requested a review from nateinaction July 26, 2025 18:39
@Mikefly123 Mikefly123 merged commit 04dfa3f into main Jul 26, 2025
5 checks passed
@Mikefly123 Mikefly123 deleted the mcp9808-manager branch July 26, 2025 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Vibe Coded This was written mostly by AI with human oversight
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants