Add I2C bus frequency readback property#278
Open
voldemort9999 wants to merge 2 commits intofossasia:mainfrom
Open
Add I2C bus frequency readback property#278voldemort9999 wants to merge 2 commits intofossasia:mainfrom
voldemort9999 wants to merge 2 commits intofossasia:mainfrom
Conversation
Store the actual I2C bus frequency after configuration so that sensors can check if the configured baudrate is compatible. Fixes fossasia#195
Reviewer's GuideAdds a stored I2C bus frequency to the core I2C primitive and exposes it as a read-only property on I2CMaster, ensuring the value reflects the actual hardware frequency derived from the configured BRG register, with comprehensive unit tests validating behavior and boundaries. Sequence diagram for I2CMaster configuration and frequency readbacksequenceDiagram
actor Driver
participant I2CMaster
participant _I2CPrimitive
participant ConnectionHandler as ConnectionHandler
Driver->>I2CMaster: __init__()
I2CMaster->>_I2CPrimitive: __init__(device)
_I2CPrimitive->>ConnectionHandler: autoconnect() (if device is None)
_I2CPrimitive-->>I2CMaster: _device, _running=False, _mode=None, _frequency=None
I2CMaster->>_I2CPrimitive: _init()
_I2CPrimitive->>ConnectionHandler: send_byte(CP.I2C_HEADER)
I2CMaster->>I2CMaster: configure(125e3)
I2CMaster->>_I2CPrimitive: _configure(125e3)
_I2CPrimitive->>_I2CPrimitive: compute brgval from frequency
_I2CPrimitive->>ConnectionHandler: send_byte(CP.I2C_CONFIG)
_I2CPrimitive->>ConnectionHandler: send_int(brgval)
_I2CPrimitive->>ConnectionHandler: get_ack()
_I2CPrimitive->>_I2CPrimitive: _frequency = _get_i2c_frequency(brgval)
Driver->>I2CMaster: configure(400e3)
I2CMaster->>_I2CPrimitive: _configure(400e3)
_I2CPrimitive->>_I2CPrimitive: compute brgval, update _frequency
Driver->>I2CMaster: frequency
I2CMaster-->>Driver: _frequency (actual hardware frequency)
Updated class diagram for I2C primitives and I2CMaster frequency propertyclassDiagram
class ConnectionHandler
class _I2CPrimitive {
- ConnectionHandler _device
- bool _running
- any _mode
- float _frequency
+ _I2CPrimitive(device ConnectionHandler)
+ _init() void
+ _configure(frequency float) void
+ _get_i2c_frequency(brgval int) float
}
class I2CMaster {
+ I2CMaster(device ConnectionHandler)
+ frequency float
+ configure(frequency float) void
}
_I2CPrimitive --> ConnectionHandler : uses
I2CMaster --> _I2CPrimitive : composes/extends (implementation detail)
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
I2CMaster.frequencydocstring advertises afloat, but_frequencycan beNonebeforeconfigure()runs in some code paths; consider either initializing_frequencyto a sensible default or updating the documentation/typing to reflect the possibleNonevalue. - Since
_frequencyis managed in_I2CPrimitive, you might consider exposing a commonfrequencyproperty on_I2CPrimitiverather than only onI2CMaster, so that consumers usingI2CSlave(or other subclasses) can access the configured bus speed in a consistent way.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `I2CMaster.frequency` docstring advertises a `float`, but `_frequency` can be `None` before `configure()` runs in some code paths; consider either initializing `_frequency` to a sensible default or updating the documentation/typing to reflect the possible `None` value.
- Since `_frequency` is managed in `_I2CPrimitive`, you might consider exposing a common `frequency` property on `_I2CPrimitive` rather than only on `I2CMaster`, so that consumers using `I2CSlave` (or other subclasses) can access the configured bus speed in a consistent way.
## Individual Comments
### Comment 1
<location path="tests/test_i2c_frequency.py" line_range="218-226" />
<code_context>
+ master.configure(min_freq)
+ assert master.frequency == pytest.approx(min_freq)
+
+ def test_max_valid_frequency(self, master):
+ """Test with a high frequency whose brgval equals MIN_BRGVAL."""
+ # _get_i2c_frequency(MIN_BRGVAL) gives the theoretical max, but due
+ # to int() truncation in _get_i2c_brgval, that value may not
+ # round-trip. Instead, find a frequency that computes to MIN_BRGVAL.
+ max_brgval_freq = _I2CPrimitive._get_i2c_frequency(
+ _I2CPrimitive._MIN_BRGVAL + 1
+ )
+ master.configure(max_brgval_freq)
+ assert master.frequency is not None
+ assert master.frequency > 0
+
+ def test_just_below_min_frequency_raises(self, master):
</code_context>
<issue_to_address>
**suggestion (testing):** The max-frequency boundary test could assert a stronger condition than just `frequency > 0`.
In `test_max_valid_frequency`, you derive `max_brgval_freq` from `_MIN_BRGVAL + 1` but only check that `master.frequency` is positive. To better validate this boundary, consider also asserting that `_I2CPrimitive._get_i2c_brgval(max_brgval_freq)` returns the expected BRG value (e.g. `_MIN_BRGVAL + 1`), and that `master.frequency == pytest.approx(_I2CPrimitive._get_i2c_frequency(_I2CPrimitive._get_i2c_brgval(max_brgval_freq)))`, mirroring the minimum-frequency test and confirming the upper edge is handled correctly.
```suggestion
# _get_i2c_frequency(MIN_BRGVAL) gives the theoretical max, but due
# to int() truncation in _get_i2c_brgval, that value may not
# round-trip. Instead, find a frequency that computes to MIN_BRGVAL.
max_brgval_freq = _I2CPrimitive._get_i2c_frequency(
_I2CPrimitive._MIN_BRGVAL + 1
)
# Configure with a frequency that should map to BRG = MIN_BRGVAL + 1
master.configure(max_brgval_freq)
# Verify that the BRG value computed from this frequency is as expected
computed_brgval = _I2CPrimitive._get_i2c_brgval(max_brgval_freq)
assert computed_brgval == _I2CPrimitive._MIN_BRGVAL + 1
# And that the configured frequency matches the "round-tripped" frequency
roundtrip_freq = _I2CPrimitive._get_i2c_frequency(computed_brgval)
assert master.frequency == pytest.approx(roundtrip_freq)
```
</issue_to_address>
### Comment 2
<location path="tests/test_i2c_frequency.py" line_range="133-142" />
<code_context>
+# ============================================================
+# Test 5: frequency unchanged on invalid configure (ValueError)
+# ============================================================
+class TestFrequencyOnError:
+
+ def test_frequency_unchanged_on_invalid_frequency(self, master):
+ """If configure() raises ValueError, frequency should remain unchanged."""
+ master.configure(125e3)
+ original_freq = master.frequency
+
+ # Too low frequency should raise ValueError.
+ with pytest.raises(ValueError):
+ master.configure(1) # Way too low
+
+ # Frequency should be unchanged.
+ assert master.frequency == original_freq
+
+ def test_frequency_unchanged_on_too_high_frequency(self, master):
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for how `_frequency` behaves if the underlying device NACKs the configuration.
Current tests only cover `ValueError` from invalid frequency values and verify `_frequency` is unchanged. There’s another failure mode where the device NACKs (e.g., `device.get_ack()` returns 0 or raises); in that case `_frequency` should also stay unchanged. Please add a test that uses a `mock_device` with a failing `get_ack` and asserts that `configure()` handles the failure and does not update `master.frequency`.
Suggested implementation:
```python
# Test 5: frequency unchanged on invalid configure (ValueError)
# ============================================================
class TestFrequencyOnError:
def test_frequency_unchanged_on_invalid_frequency(self, master):
"""If configure() raises ValueError, frequency should remain unchanged."""
master.configure(125e3)
original_freq = master.frequency
# Too low frequency should raise ValueError.
with pytest.raises(ValueError):
master.configure(1) # Way too low
# Frequency should be unchanged.
assert master.frequency == original_freq
def test_frequency_unchanged_on_too_high_frequency(self, master):
"""If configure() raises ValueError for too-high freq, frequency unchanged."""
master.configure(125e3)
original_freq = master.frequency
# Too high frequency should raise ValueError.
with pytest.raises(ValueError):
master.configure(100e6) # Way too high (100 MHz)
assert master.frequency == original_freq
=======
# ============================================================
# Test 5: frequency unchanged on invalid configure (ValueError)
# ============================================================
class TestFrequencyOnError:
def test_frequency_unchanged_on_invalid_frequency(self, master):
"""If configure() raises ValueError, frequency should remain unchanged."""
master.configure(125e3)
original_freq = master.frequency
# Too low frequency should raise ValueError.
with pytest.raises(ValueError):
master.configure(1) # Way too low
# Frequency should be unchanged.
assert master.frequency == original_freq
def test_frequency_unchanged_on_too_high_frequency(self, master):
"""If configure() raises ValueError for too-high freq, frequency unchanged."""
master.configure(125e3)
original_freq = master.frequency
# Too high frequency should raise ValueError.
with pytest.raises(ValueError):
master.configure(100e6) # Way too high (100 MHz)
assert master.frequency == original_freq
def test_frequency_unchanged_on_device_nack(self, master, mock_device, monkeypatch):
"""
If the underlying device NACKs during configure(), frequency should remain unchanged.
This simulates a failure where device.get_ack() indicates a NACK (e.g., returns 0).
"""
# Start from a known good configuration.
master.configure(125e3)
original_freq = master.frequency
# Force the device to NACK any subsequent configuration attempt.
monkeypatch.setattr(mock_device, "get_ack", lambda *args, **kwargs: 0)
# configure() should handle the NACK as a failure and not update the frequency.
with pytest.raises(Exception):
master.configure(400e3)
# Frequency must remain unchanged after the failed configure().
assert master.frequency == original_freq
def test_frequency_unchanged_when_get_ack_raises(self, master, mock_device, monkeypatch):
"""
If device.get_ack() raises during configure(), frequency should remain unchanged.
This covers the failure mode where the low-level I/O layer throws instead of
returning a NACK code.
"""
master.configure(125e3)
original_freq = master.frequency
def _failing_get_ack(*args, **kwargs):
raise IOError("Simulated I2C NACK/IO failure")
monkeypatch.setattr(mock_device, "get_ack", _failing_get_ack)
with pytest.raises(IOError):
master.configure(400e3)
assert master.frequency == original_freq
```
If `configure()` currently does not surface NACKs as `Exception`/`IOError` in the ways assumed here (e.g., if it uses a custom exception type), adjust the `pytest.raises(...)` expectations to match the actual exception class(es) used in your driver. Also ensure the `mock_device` fixture is the same object used internally by `master`; if not, you may need to patch `master._device` (or equivalent) instead of `mock_device` directly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
While going through the codebase I noticed that
I2CMaster.configure()sends the frequency to the hardware but doesn't keep track of what was set. This makes it impossible for sensor drivers to verify whether the bus speed is within their supported range — for example, the MLX90614 only supports up to 100 kHz SMBus, but there's currently no way to check this programmatically.This PR adds a frequency property to I2CMaster that reflects the actual configured bus speed.
Closes #195
Changes
_I2CPrimitive.__init__(initialized asNone)Usage
Tests
Added tests/test_i2c_frequency.py with 18 unit tests covering default frequency, reconfigurations, brgval rounding, read-only enforcement, boundary conditions, and error handling. All tests use
unittest.mockand run without hardware.Summary by Sourcery
Add an I2C bus frequency readback on I2CMaster so code can query the actual configured bus speed.
New Features:
Enhancements:
Tests: