Skip to content
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

boards/nrf52dk: Minimal Arduino pinout support #20286

Merged
merged 3 commits into from
Jan 23, 2024

Conversation

chrysn
Copy link
Member

@chrysn chrysn commented Jan 22, 2024

Contribution description

This defines the Arduino GPIO pins, sets the shield I2C bus and the form factor.

Testing procedure

Run tests/periph/selftest_shield with an nrf52dk and a Peripheral Selftest Shield in "all off, D-3-4 on" configuration.

Issues/PRs references

This simplifies testing #20282

PR status

I'm keeping this as Draft PR because I'm still looking at whether there are cheap extras to enable (maybe SPI bus, maybe the ADC pins). If I find none, this will be changed to "ready for review" without further comment.

@chrysn chrysn requested a review from maribu January 22, 2024 22:50
@chrysn chrysn added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: boards Area: Board ports labels Jan 22, 2024
@chrysn
Copy link
Member Author

chrysn commented Jan 22, 2024

On the good side: I found something that can be added easily.

Now, test with jumpers 5, 4 and 2 set (D7-CS, D3-D4, Side SPI).

On the bad side, there seems to be somethign wrong with the nRF52 SPI driver, as the test's main.c needs this patch to run through successfully:

--- a/tests/periph/selftest_shield/main.c
+++ b/tests/periph/selftest_shield/main.c
@@ -857,4 +857,5 @@ static bool periph_spi_rxtx_test(spi_t bus, spi_mode_t mode, spi_clk_t clk,
         }
         uint8_t received = spi_transfer_byte(bus, cs, true, i);
+        received = spi_transfer_byte(bus, cs, true, i);
         uint16_t stop = 0;
         if (IS_USED(MODULE_PERIPH_TIMER)) {

Without this, every other (literally: one out of two in alternation) SPI byte transfer will not read back the written value, but the value written in the last iteration. I suspected premature resumption (like, maybe there's some register buffering and writes are too fast), but large delays in the outer loop don't fix it either.

If all other properties of this PR are to the reviewers' satisfaction, I'd prefer to have this merged, and then start digging for where the SPI goes wrong -- after all, this PR doesn't cause the trouble, it just makes them testable.

@chrysn chrysn marked this pull request as ready for review January 22, 2024 23:47
@chrysn chrysn requested a review from aabadie as a code owner January 22, 2024 23:47
@chrysn
Copy link
Member Author

chrysn commented Jan 22, 2024

I'm observing that doing 2-byte writes also fixes the many failures:

@@ -857,3 +857,6 @@ static bool periph_spi_rxtx_test(spi_t bus, spi_mode_t mode, spi_clk_t clk,
         }
-        uint8_t received = spi_transfer_byte(bus, cs, true, i);
+        const uint8_t outbuf[2] = {i, 0xff};
+        uint8_t inbuf[2] = {0x55, 0x55};
+        spi_transfer_bytes(bus, cs, true, &outbuf, &inbuf, 2);
+        uint8_t received = inbuf[0];
         uint16_t stop = 0;

CC'ing @bergzand who contributed the SPI driver in #14057, including a workaround for single-byte transfers which this test uses.

@chrysn chrysn added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 23, 2024
@riot-ci
Copy link

riot-ci commented Jan 23, 2024

Murdock results

✔️ PASSED

7452a61 fixup! boards/nrf52dk: Minimal Arduino pinout support

Success Failures Total Runtime
1128 0 1128 03m:46s

Artifacts

Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

Thx :)

@maribu maribu enabled auto-merge January 23, 2024 05:27
@maribu maribu added this pull request to the merge queue Jan 23, 2024
Merged via the queue into RIOT-OS:master with commit e5cb676 Jan 23, 2024
26 of 27 checks passed
@chrysn chrysn deleted the nrf52dk-arduino branch January 23, 2024 11:26
@chrysn
Copy link
Member Author

chrysn commented Jan 23, 2024

Brief follow-up on the SPI trouble: I have no clue how that went wrong, but I can't reproduce the SPI trouble any more.

All shiny, tests pass.

@MrKevinWeiss MrKevinWeiss added this to the Release 2024.01 milestone Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants