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

SPI access in LoRa-e5 and im880b? #19025

Closed
jmfriedt opened this issue Dec 8, 2022 · 17 comments
Closed

SPI access in LoRa-e5 and im880b? #19025

jmfriedt opened this issue Dec 8, 2022 · 17 comments
Assignees
Labels
Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Comments

@jmfriedt
Copy link
Contributor

jmfriedt commented Dec 8, 2022

When compiling the tests/periph_spi example for LoRa-e5 board or im880b, the
execution of

> init 1 0 0
Trying to initialize SPI_DEV(1): mode: 0, clk: 0, cs_port: 0, cs_pin: 0
Note: Failed assertion (crash) means configuration not supported

in the provided shell fails, preventing sending messages on the MOSI and CLK pins. This
used to work with tag 6bf6b6b where the expected result is

> init 1 0 0
SPI_DEV(1) initialized: mode: 0, clk: 0, cs_port: 0, cs_pin: 0
> 

and expected signals are observed on the oscilloscope on MOSI and CLK pins. Has the API changed to initialize the SPI bus?
Thanks

@jdavid
Copy link
Contributor

jdavid commented Dec 26, 2022

I can reproduce the issue with the LoRa-E5 board. But I could not test commit 6bf6b6b since at that time boards/lora-e5-dev did not exist yet.

@jmfriedt have you done a git bisect to identify with which commit it stopped working?
If you could do that I will try to do a pull request.

@benpicco benpicco added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Jan 2, 2023
@benpicco
Copy link
Contributor

benpicco commented Jan 2, 2023

This used to work with tag 6bf6b6b

That's a good starting point!
Can you do a

git bisect start
git bisect bad master
git bisect good 6bf6b6be6c4723b49f62550a35111e57b7426aa4 

to find the offending commit?

@jmfriedt
Copy link
Contributor Author

jmfriedt commented Jan 3, 2023

Part of the issue is that RIOT developers are providing patches that break the whole system until the last update is provided, so the first step of bisect already fails with

...
Bisecting: 5604 revisions left to test after this (roughly 13 steps)
[96eb2c50506dca51270039d2cb8ebb6139199977] Merge pull request #16731 from JKRhb/dhcp-stateless
...
/usr/lib/gcc/arm-none-eabi/10.3.1/../../../arm-none-eabi/bin/ld: /tmp/RIOT/tests/periph_spi/bin/im880b/periph_common/gpio_util.o: in function `gpio_util_shiftin':
/tmp/RIOT/drivers/periph_common/gpio_util.c:15: multiple definition of `gpio_util_shiftin'; /tmp/RIOT/tests/periph_spi/bin/im880b/periph_common/gpio.o:/tmp/RIOT/drivers/periph_common/gpio.c:13: first defined here
collect2: error: ld returned 1 exit status
make: *** [/tmp/RIOT/tests/periph_spi/../../Makefile.include:686: /tmp/RIOT/tests/periph_spi/bin/im880b/tests_periph_spi.elf] Error 1

so the final set of commits must be identified before the compilation can be completed and tested.

Thanks.

@benpicco
Copy link
Contributor

benpicco commented Jan 3, 2023

I hope there are not too much of those - you can use git bisect skip to try the next commit.

@jmfriedt
Copy link
Contributor Author

jmfriedt commented Jan 3, 2023

A lot of different issues seem to overlap. So far I have on the im880b

7ddfb3de4222fe0f641cf36f29339632e9f6bcbf fail       Sat Jun 4 11:55:44 2022
48fc63e0ae60aec05c16cf641b486b1affb58114 no display
bd37457e14fa424a85d2a9f0b040928db6c47001 no display
3a8f74da0cb6ca20266fd2a801c7063e2417fbdc no display Wed Nov 4 23:00:51 2020
4034f9616983d8c653ded05476e93ee5525c6320 OK         Wed May 6  23:46:43 2020
3e8d6388f8706ce15fc8d81ea440fb1555da96b6 OK         Fri Aug 21 22:20:42 2020
daecd9c607d766a3f269d0bde389fbb19e250b85 OK         Tue Aug 25 16:01:25 2020

with fail meaning the SPI port will not initialize, no display meaning no output on the RS232 port (either port initialization failure or clock initialization failure since not even the RIOT prompt is displayed) and OK meaning SPI port initialization functional. I am not sure how to separate the lack of display from the non-functional SPI !

I cannot bisect much further as I always get stuck with a version of RIOT that is unable to compile with the proposed hash

$RIOT/tests/periph_spi/bin/im880b/stm32_vectors/vectors_l1.o: in function `dummy_handler':
$RIOT/cpu/stm32/vectors/vectors_l1.c:28: multiple definition of `dummy_handler'; 
$RIOT/tests/periph_spi/bin/im880b/stm32_vectors/vectors_l1.o:/tmp/RIOT/cpu/stm32/vectors/vectors_l1.c:85: multiple definition of `vector_cpu'; $RIOT/tests/periph_spi/bin/im880b/stm32_vectors/STM32L151xB.o:/tmp/RIOT/cpu/stm32/vectors/STM32L151xB.c:58: first defined here

so the proposed hash above are the only ones that lead to a successful compilation (not even talking of a functional system!)

@kaspar030
Copy link
Contributor

Part of the issue is that RIOT developers are providing patches that break the whole system until the last update is provided, so the first step of bisect already fails with

Do you mean that in a PR, the individual commits are not necessarily buildable?
That's a trade-off we chose to make.
You can bisect at merge commit granularity using git bisect --first-parent ...

@jmfriedt
Copy link
Contributor Author

jmfriedt commented Jan 9, 2023

Not sure what I am doing wrong, but already the first compilation fails when
$ git bisect start --first-parent
$ git bisect good 6bf6b6b
$ git bisect bad master
$ cd tests/periph_spi
$ make BOARD=im880b
[...]
...RIOT/sys/shell/shell.c:120:42: error: the comparison will always evaluate as 'true' for the address of '_shell_command_list' will never be NULL [-Werror=address]
120 | if (handler == NULL && _builtin_cmds != NULL) {
| ^~
In file included from /tmp/RIOT/sys/shell/shell.c:41:
/tmp/RIOT/sys/include/shell_commands.h:44:30: note: '_shell_command_list' declared here
44 | extern const shell_command_t _shell_command_list[];
| ^~~~~~~~~~~~~~~~~~~
/tmp/RIOT/sys/shell/shell.c: In function 'print_help':
/tmp/RIOT/sys/shell/shell.c:155:23: error: the comparison will always evaluate as 'true' for the address of '_shell_command_list' will never be NULL [-Werror=address]
155 | if (_builtin_cmds != NULL) {
| ^~
...RIOT/sys/include/shell_commands.h:44:30: note: '_shell_command_list' declared here
44 | extern const shell_command_t _shell_command_list[];

could be the newer gcc (
$ arm-none-eabi-gcc --version
arm-none-eabi-gcc (15:12.2.rel1-1) 12.2.1 20221205
) is more cautious than the one used at the time.
Thanks.

@jdavid
Copy link
Contributor

jdavid commented Jan 13, 2023

Yes it looks like a new warning in GCC 12.

I think this should work, to ignore address warnings:

CFLAGS="-Wno-error=address" BOARD=im880b  make -C tests/periph_spi

@jmfriedt
Copy link
Contributor Author

jmfriedt commented Jan 15, 2023

Thank you all for your help. I reached a functional version with the following bisect sequence

174ee0175fc6cff42de72bd82a5f864dbb0f636c no display Thu Sep 9 13:01:49 2021
582f8d84303054e0ae8228b12199d05a02607581 no display Wed Jan 13 13:28:06 2021
f46572a3085090173a00ccd7185f3df0940dda39 no display Tue Oct 27 10:32:18 2020
f862166cbbb7e7f7b2c70971b168036d7badecb7 no display Thu Sep 24 14:20:26 2020
ba67f6aa274a7ecad8c56d543e089f6d21efd5d4 good       Thu Sep 3 22:09:27 2020
cc735805c8ccf3a7a2e95fd809727d8e1b5faf1b good       Fri Sep 11 10:45:47 2020
2e89cf3398dfaf446fecabfafffe85f922310887 good       Thu Sep 17 16:29:13 2020
4db66d11105245ea7db34ce0b3dee3a9faf7db81 good       Tue Sep 22 21:41:13 2020
711ea275e93f483f1a2da33a3d3dbf12a2fe662a no display Wed Sep 23 13:37:36 2020
0806b4b17389cb35cbeac9ef91b231a561fb98e3 no display Wed Sep 23 09:52:00 2020
195b7e6e16cbd8ee09271388c007ed97ed3c0a0c no display Wed Sep 23 09:30:19 2020  boards/stm32l0l1: rework clock initialization and configuration
dc5631e8e3e98d862bb599737e4f6651ed965c48 good       Wed Sep 23 09:25:21 2020

which I am afraid is conflicting with #18295 when multiple im880b boards are just not communicating over RS232 after reset (I was given a dozen im880b boards with half working through RS232/stm32flash programming and the other half working when programmed by ST-Link/openocd ... we suspect clock initialization issues but have not solved this problem either, preventing the identification of the faulty SPI CS# definition on this board). Indeed the offending commit 195b7e6 mentions rework of the stm32l0l1 clock configuration and is dated one week later than the initially identified c14d7ec which was the focus of #18295

Thanks.

@jdavid
Copy link
Contributor

jdavid commented Jan 16, 2023

If I follow dc5631e was the last good commit, then 195b7e6 (PR #14945) broke RS232. Later this was fixed, but SPI didn't work. So we cannot identify which merge broke SPI. Maybe can we narrow it? Which was the first merge where RS232 started working again?

@jmfriedt
Copy link
Contributor Author

jmfriedt commented Jan 16, 2023

OK I give up on the im880b, too many issues at the same time. I just stumbled upon a ST B-L072Z-LRWAN1 LoRa discovery whose communication remains functional to this day. By compiling with

CFLAGS="-Wno-error=address" BOARD=b-l072z-lrwan1  make -C tests/periph_spi

and transfering the binary file through the mass storage interface, the following bisect sequence

master                                   bad
f7bed004ff5529a84fdc63e11ebc5b91e09a4327 bad  Sun Nov 7 07:52:16 2021
fbe6748c57a72c04c081da0d834bd44a0e1eeafb good Thu Feb 18 09:24:33 2021
08671255bea508fb6de7a752c22509cb1eb048ce good Wed Jun 23 16:30:17 2021
174ee0175fc6cff42de72bd82a5f864dbb0f636c bad  Thu Sep 9 13:01:49 2021
6e0c8db99f9065c06d9455282bca9efca4471ca9 good Sun Aug 8 02:28:12 2021
149de73160f68f80257f7db57657e267ad5c98fa good Thu Aug 26 14:37:58 2021
2bf1202fb9812c6bc8a7aa63720b17c828d944ad bad  Thu Sep 2 20:54:01 2021
db16cafe045627e4fab337dbf526ae0d22843455 good Wed Sep 1 11:58:00 2021
65d717f5a082a6fb8a20eb43032b816103e7b3c2 bad  Thu Sep 2 08:54:53 2021
df205bc4526aa2d1bbf52594ec22d96b96ed4359 good Wed Sep 1 22:11:39 2021
a1cbcc9edef94c46ccb8a35615049a0e61d80868 bad  Thu Sep 2 08:50:56 2021
550565f05250876c5c64fda927602a341e283320 good Wed Sep 1 22:28:14 2021

where "good" means the SPI init 1 0 0 is acknowledged and bad means I get

> init 1 0 0
Trying to initialize SPI_DEV(1): mode: 0, clk: 0, cs_port: 0, cs_pin: 0
Note: Failed assertion (crash) means configuration not supported

the final answer is

a1cbcc9edef94c46ccb8a35615049a0e61d80868 is the first bad commit
commit a1cbcc9edef94c46ccb8a35615049a0e61d80868
Merge: 550565f052 2efc50be97
Author: Francisco <femolina@uc.cl>
Date:   Thu Sep 2 08:50:56 2021 +0200

    Merge pull request #15902 from maribu/spi-api-change-1
    
    drivers/periph_spi: let spi_acquire return void

@jdavid
Copy link
Contributor

jdavid commented Jan 19, 2023

Reading the source code I realise that the code actually works, the following message is merely informative:

Note: Failed assertion (crash) means configuration not supported

This is the source code:

        printf("Trying to initialize SPI_DEV(%i): mode: %i, clk: %i, cs_port: %i, cs_pin: %i\n",
               dev, mode, clk, port, pin);
        puts("Note: Failed assertion (crash) means configuration not supported");
        spi_acquire(spiconf.dev, spiconf.cs, spiconf.mode, spiconf.clk);
        spi_release(spiconf.dev);

I think there should be another puts(..) at the end saying the operation was successful, and the note should rephrased, something like:

(if below the program crashes with a failed assertion, then it means the configuration is not supported)

I'll do a pull request.

However, the fact that the test pass doesn't mean that SPI works (right?). In the original description @jmfriedt says:

and expected signals are observed on the oscilloscope on MOSI and CLK pins.

I don't have an oscilloscope to test. Then, how could I verify whether SPI really works or not?

(For context what I want is to fix issue #18975 , thought that may be an SPI problem or something else).

jdavid added a commit to spectraphilic/RIOT that referenced this issue Jan 19, 2023
The way it's now it's easy to interpret that there was an error, when
really there was not. As seen in issue RIOT-OS#19025 quite some time can be
spent following a wrong clue.
@benpicco
Copy link
Contributor

However, the fact that the test pass doesn't mean that SPI works (right?)

The bench command only checks how long individual SPI operations take.
If you want to test send and receive, connect MISO and MOSI with a Jumper/Wire and use the send command. You should receive the same bytes that you are sending.

This does of course not check if the clock signal has the correct frequency.

@jdavid
Copy link
Contributor

jdavid commented Jan 19, 2023

Well, I can confirm SPI works in the lora-e5 board, I got the sdcard working, see comment #18975 (comment)

@jmfriedt
Copy link
Contributor Author

jmfriedt commented Jan 19, 2023

Connecting as loopback MOSI to MISO of the im880b (P7 to P8) does not return the transmitted byte using send of the tests/periph_spi application after either init 1 0 0 or init 0 0 0 (nor do signals appear on the scope) i.e either using SPI0 or SPI1.

bors bot added a commit that referenced this issue Jan 19, 2023
19156: core/compiler_hints: add likely() / unlikely() hints r=kfessel a=benpicco



19174: tests/periph_spi clearly say when init succeeds r=benpicco a=jdavid

When the `tests/periph_spi` program succeeds the output can be interpreted as an error happened.
This PR makes it clearer when it does succeed.

### Contribution description

In `tests/periph_spi`:

- Explicitely say that the init operation was successful
- Rephrase the note to avoid misinterpretations 


### Testing procedure

Run the `tests/periph_spi` program.
There is not much to test, just to verify the output, should be like this:

```
2023-01-19 10:42:33,768 # Trying to initialize SPI_DEV(1): mode: 0, clk: 0, cs_port: 0, cs_pin: 0
2023-01-19 10:42:33,777 # (if below the program crashes with a failed assertion, then it means the configuration is not supported)
2023-01-19 10:42:33,779 # Success.
```


### Issues/PRs references

Issue #19025

Co-authored-by: Benjamin Valentin <benjamin.valentin@ml-pa.com>
Co-authored-by: J. David Ibáñez <jdavid.ibp@gmail.com>
Einhornhool pushed a commit to Einhornhool/RIOT that referenced this issue Jan 24, 2023
The way it's now it's easy to interpret that there was an error, when
really there was not. As seen in issue RIOT-OS#19025 quite some time can be
spent following a wrong clue.
dylad pushed a commit to dylad/RIOT that referenced this issue Feb 3, 2023
The way it's now it's easy to interpret that there was an error, when
really there was not. As seen in issue RIOT-OS#19025 quite some time can be
spent following a wrong clue.
@maribu
Copy link
Member

maribu commented May 11, 2023

Just to be sure: SPI_DEV(0) of the im880b is internally connected to the LoRa chip on the PCB. To do an actual loopback check, one would have to cut the traces to the LoRa chip and solder on a wire in place of the traces, which would basically destroy the PCB.

For SPI_DEV(1) of the im880b and a wire connecting the pins 12 (PB14) and 13 (PB15) I do get:

~/Repos/software/RIOT/tests/periph/spi $ make BOARD=im880b PROGRAMMER=stm32flash STM32FLASH_RESET=1 term
/home/maribu/Repos/software/RIOT/dist/tools/pyterm/pyterm -p "/dev/ttyUSB0" -b "115200" --set-rts 1 --set-dtr 0 
2023-05-11 14:31:34,095 # Connect to serial port /dev/ttyUSB0
Welcome to pyterm!
Type '/exit' to exit.
2023-05-11 14:31:35,134 # main(): This is RIOT! (Version: 2023.07-devel-276-g0a7bc)
2023-05-11 14:31:35,134 # Manual SPI peripheral driver test (see README.md)
2023-05-11 14:31:35,135 # There are 2 SPI devices configured for your platform.
> init 1 0 0
2023-05-11 14:31:39,261 # init 1 0 0
2023-05-11 14:31:39,268 # Trying to initialize SPI_DEV(1): mode: 0, clk: 0, cs_port: 0, cs_pin: 0
2023-05-11 14:31:39,277 # (if below the program crashes with a failed assertion, then it means the configuration is not supported)
2023-05-11 14:31:39,278 # Success.
> send fooo
2023-05-11 14:32:03,538 # send fooo
2023-05-11 14:32:03,540 # Sent bytes
2023-05-11 14:32:03,542 #    0    1    2    3 
2023-05-11 14:32:03,544 #   0x66 0x6f 0x6f 0x6f
2023-05-11 14:32:03,546 #     f    o    o    o 
2023-05-11 14:32:03,546 # 
2023-05-11 14:32:03,547 # Received bytes
2023-05-11 14:32:03,549 #    0    1    2    3 
2023-05-11 14:32:03,551 #   0xe6 0x66 0x6f 0x6f
2023-05-11 14:32:03,553 #    ??    f    o    o 

This looks just fine to me (tested on current master at commit 0a7bc1a45c187e1e0b8abe93f0c6a52fadf3f44c).

Also, examples/gnrc_networking seems to correctly interact with the LoRa chip via SPI (after disabling netstats to fit into RAM):

2023-05-11 14:39:55,860 # main(): This is RIOT! (Version: 2023.07-devel-276-g0a7bc)
2023-05-11 14:39:55,860 # RIOT network stack example application
2023-05-11 14:39:55,860 # All up, running the shell now
> ifconfig
2023-05-11 14:39:58,791 # ifconfig
2023-05-11 14:39:58,799 # Iface  5  Frequency: 868299987Hz  RSSI: -139  BW: 125kHz  SF: 7  CR: 4/5 
2023-05-11 14:39:58,804 #            TX-Power: 14dBm  State: SLEEP 
2023-05-11 14:39:58,809 #           L2-PDU:2047  MTU:255  HL:64  RTR  
2023-05-11 14:39:58,811 #           RTR_ADV  
2023-05-11 14:39:58,815 #           Link type: wireless
2023-05-11 14:39:58,818 #           inet6 group: ff02::2
2023-05-11 14:39:58,821 #           inet6 group: ff02::1
2023-05-11 14:39:58,825 #           inet6 group: ff02::1a
2023-05-11 14:39:58,826 #           

Can you confirm that there is an actual issue?

@maribu
Copy link
Member

maribu commented May 17, 2023

Since @jdavid got SPI according to #18975 running on the lore-e5 board and I got it working on the im880b board, I'd say this can be closed.

Please feel free to re-open if you disagree. Or to post questions here if you need assistance.

@maribu maribu closed this as completed May 17, 2023
zhaolanhuang pushed a commit to zhaolanhuang/RIOT that referenced this issue Dec 6, 2023
The way it's now it's easy to interpret that there was an error, when
really there was not. As seen in issue RIOT-OS#19025 quite some time can be
spent following a wrong clue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

No branches or pull requests

7 participants