Fix: Update SPI driver for firmware v3 by using _BURST commands#275
Open
TejasAnalyst wants to merge 4 commits intofossasia:mainfrom
Open
Fix: Update SPI driver for firmware v3 by using _BURST commands#275TejasAnalyst wants to merge 4 commits intofossasia:mainfrom
TejasAnalyst wants to merge 4 commits intofossasia:mainfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates the SPI driver to use firmware v3 *_BURST transfer commands and removes now-unnecessary START/STOP bracketing in SPI operations, while adjusting SPI/UART tests to use fixed PWM frequencies instead of class-level instance properties. Sequence diagram for SPI transfer8 using firmware v3 burst commandsequenceDiagram
participant Client
participant SPI as _SPIPrimitive
participant FirmwareV3
Client->>SPI: transfer8(data)
activate SPI
SPI->>SPI: _transfer(data, 8)
SPI->>FirmwareV3: SEND_SPI8_BURST(data)
FirmwareV3-->>SPI: data_in
SPI-->>Client: data_in
deactivate SPI
Updated class diagram for SPI primitive with burst commandsclassDiagram
class _SPIPrimitive {
-_TRANSFER_COMMANDS_MAP: dict
-_INTEGER_TYPE_MAP: dict
+transfer8(data: int) int
+transfer16(data: int) int
+transfer8_bulk(data: List[int]) List[int]
+transfer16_bulk(data: List[int]) List[int]
+read8() int
+read16() int
+read8_bulk(data_to_read: int) List[int]
+read16_bulk(data_to_read: int) List[int]
-_transfer(data: int, bits: int) int
-_transfer_bulk(data: List[int], bits: int) List[int]
-_read(bits: int) int
-_read_bulk(data_to_read: int, bits: int) List[int]
}
File-Level Changes
Assessment against linked issues
Possibly 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:
- If
_start()and_stop()are no longer used anywhere after switching to the*_BURSTcommands, consider removing these methods (and any related state) to simplify the SPI driver and avoid dead code. - You now have hard-coded frequency literals in multiple places in the tests; consider centralizing these values (e.g., as module-level constants or a helper) so future changes to the test frequencies only need to be made in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- If `_start()` and `_stop()` are no longer used anywhere after switching to the `*_BURST` commands, consider removing these methods (and any related state) to simplify the SPI driver and avoid dead code.
- You now have hard-coded frequency literals in multiple places in the tests; consider centralizing these values (e.g., as module-level constants or a helper) so future changes to the test frequencies only need to be made in one place.
## Individual Comments
### Comment 1
<location> `tests/test_spi.py:34` </location>
<code_context>
SPIMaster._secondary_prescaler = SPRE = 0
-PWM_FERQUENCY = SPIMaster._frequency * 2 / 3
+# Static value 100kHz used because instance property '_frequency' cannot be accessed on the class.
+PWM_FREQUENCY = 100000.0
MICROSECONDS = 1e-6
RELTOL = 0.05
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid hardcoding PWM frequency; derive it from the SPI master instance to keep the test aligned with implementation changes.
Hardcoding `PWM_FREQUENCY = 100000.0` means the test may no longer reflect the real SPI clock if `_frequency` or its configuration changes. Instead, compute the PWM frequency in a fixture that has access to the `SPIMaster` instance (e.g., make `la` depend on a `spi_master` fixture and derive `pwm_frequency = spi_master._frequency * 2 / 3`, or use a public API if one exists) before calling `pwm.generate(...)`. This keeps the test coupled to the actual SPI settings and preserves its value as a regression test.
Suggested implementation:
```python
SPIMaster._primary_prescaler = PPRE = 0
SPIMaster._secondary_prescaler = SPRE = 0
MICROSECONDS = 1e-6
RELTOL = 0.05
# Number of expected logic level changes.
@pytest.fixture
def la(handler: SerialHandler, spi_master: SPIMaster) -> LogicAnalyzer:
pwm = PWMGenerator(handler)
pwm_frequency = spi_master._frequency * 2 / 3
pwm.generate(SDI[1], pwm_frequency, 0.5)
return LogicAnalyzer(handler)
```
If the SPI master fixture is not currently named `spi_master` or has a different type, update the `la` fixture's parameter list to match the existing fixture name and type. Also, if `_frequency` is not accessible or a public API exists (e.g. `get_frequency()`), replace `spi_master._frequency` with the appropriate public accessor to avoid depending on private attributes.
</issue_to_address>
### Comment 2
<location> `tests/test_uart.py:18-23` </location>
<code_context>
RXD2 = "SQ1"
-PWM_FERQUENCY = UART._baudrate // 2
+# Static value 500kHz (half of default 1MHz baudrate) used as instance property cannot be accessed here.
+PWM_FREQUENCY = 500000.0
MICROSECONDS = 1e-6
RELTOL = 0.05
</code_context>
<issue_to_address>
**suggestion (testing):** Use the UART instance’s configured baudrate to derive PWM frequency instead of a hardcoded constant.
The old `PWM_FERQUENCY = UART._baudrate // 2` kept the PWM tied to the UART config; the new fixed `500000.0` will become wrong if the default baudrate changes. Consider deriving the PWM frequency from the `uart` fixture (e.g. `pwm_frequency = uart._baudrate / 2` or via a public accessor) and passing that into `pwm.generate(...)` so this integration test continues to validate the actual UART behavior rather than a hardcoded assumption.
```suggestion
RXD2 = "SQ1"
MICROSECONDS = 1e-6
RELTOL = 0.05
# Number of expected logic level changes.
@pytest.fixture
def pwm(handler: SerialHandler, uart: UART) -> None:
pwm_frequency = uart._baudrate / 2.0
pwm = PWMGenerator(handler)
pwm.generate(RXD2, pwm_frequency, 0.5)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
tests/test_spi.py
Outdated
| SPIMaster._secondary_prescaler = SPRE = 0 | ||
| PWM_FERQUENCY = SPIMaster._frequency * 2 / 3 | ||
| # Static value 100kHz used because instance property '_frequency' cannot be accessed on the class. | ||
| PWM_FREQUENCY = 100000.0 |
There was a problem hiding this comment.
suggestion (testing): Avoid hardcoding PWM frequency; derive it from the SPI master instance to keep the test aligned with implementation changes.
Hardcoding PWM_FREQUENCY = 100000.0 means the test may no longer reflect the real SPI clock if _frequency or its configuration changes. Instead, compute the PWM frequency in a fixture that has access to the SPIMaster instance (e.g., make la depend on a spi_master fixture and derive pwm_frequency = spi_master._frequency * 2 / 3, or use a public API if one exists) before calling pwm.generate(...). This keeps the test coupled to the actual SPI settings and preserves its value as a regression test.
Suggested implementation:
SPIMaster._primary_prescaler = PPRE = 0
SPIMaster._secondary_prescaler = SPRE = 0
MICROSECONDS = 1e-6
RELTOL = 0.05
# Number of expected logic level changes.
@pytest.fixture
def la(handler: SerialHandler, spi_master: SPIMaster) -> LogicAnalyzer:
pwm = PWMGenerator(handler)
pwm_frequency = spi_master._frequency * 2 / 3
pwm.generate(SDI[1], pwm_frequency, 0.5)
return LogicAnalyzer(handler)If the SPI master fixture is not currently named spi_master or has a different type, update the la fixture's parameter list to match the existing fixture name and type. Also, if _frequency is not accessible or a public API exists (e.g. get_frequency()), replace spi_master._frequency with the appropriate public accessor to avoid depending on private attributes.
Comment on lines
18
to
23
| RXD2 = "SQ1" | ||
| PWM_FERQUENCY = UART._baudrate // 2 | ||
| # Static value 500kHz (half of default 1MHz baudrate) used as instance property cannot be accessed here. | ||
| PWM_FREQUENCY = 500000.0 | ||
| MICROSECONDS = 1e-6 | ||
| RELTOL = 0.05 | ||
| # Number of expected logic level changes. |
There was a problem hiding this comment.
suggestion (testing): Use the UART instance’s configured baudrate to derive PWM frequency instead of a hardcoded constant.
The old PWM_FERQUENCY = UART._baudrate // 2 kept the PWM tied to the UART config; the new fixed 500000.0 will become wrong if the default baudrate changes. Consider deriving the PWM frequency from the uart fixture (e.g. pwm_frequency = uart._baudrate / 2 or via a public accessor) and passing that into pwm.generate(...) so this integration test continues to validate the actual UART behavior rather than a hardcoded assumption.
Suggested change
| RXD2 = "SQ1" | |
| PWM_FERQUENCY = UART._baudrate // 2 | |
| # Static value 500kHz (half of default 1MHz baudrate) used as instance property cannot be accessed here. | |
| PWM_FREQUENCY = 500000.0 | |
| MICROSECONDS = 1e-6 | |
| RELTOL = 0.05 | |
| # Number of expected logic level changes. | |
| RXD2 = "SQ1" | |
| MICROSECONDS = 1e-6 | |
| RELTOL = 0.05 | |
| # Number of expected logic level changes. | |
| @pytest.fixture | |
| def pwm(handler: SerialHandler, uart: UART) -> None: | |
| pwm_frequency = uart._baudrate / 2.0 | |
| pwm = PWMGenerator(handler) | |
| pwm.generate(RXD2, pwm_frequency, 0.5) |
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
2 participants
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.
Description
This PR updates the SPI driver to be compatible with firmware v3.
The older firmware relied on
STARTandSTOPcommands, which have been deprecated and removed in v3. This PR correctly replaces them with the new*_BURSTcommands.Changes Made
_TRANSFER_COMMANDS_MAPto useCP.SEND_SPI8_BURSTandCP.SEND_SPI16_BURST.self._start()andself._stop()calls from_transferandtransferfunctions, as the burst commands now handle the complete transaction internally.Fixes #262
Summary by Sourcery
Update SPI bus driver and related tests for compatibility with firmware v3 burst transfer commands.
Bug Fixes:
Tests: