8000 CLUE display issues with 6.0.0-alpha3 · Issue #3367 · adafruit/circuitpython · GitHub
[go: up one dir, main page]
Skip to content
8000

CLUE display issues with 6.0.0-alpha3 #3367

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.

Sign up for GitHub

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

Jump to bottom
Closed
jerryneedell opened this issue Sep 1, 2020 · 41 comments · Fixed by #3453
Closed

CLUE display issues with 6.0.0-alpha3 #3367

jerryneedell opened this issue Sep 1, 2020 · 41 comments · Fixed by #3453
Labels
bug displayio
Milestone
6.0.0

Comments

@jerryneedell
Copy link
Collaborator

With 6.0.0-Alpha3 and later, on a CLUE I am not seeing the REPL displayed on the screen.
I also tried running the 'spirit level' demo does not display anything -- no errors, just no display.
works ok with CP 5.3.1
and
6.0.0.alpha2

The REPL is woking OK in the terminal session, just not echoing to the display.
@tannewt tannewt added this to the 6.0.0 milestone Sep 1, 2020
@jerryneedell
Copy link
Collaborator Author

PyPortal works normally with 6.0.0.alpha3

@jerryneedell
Copy link
Collaborator Author
jerryneedell commented Sep 1, 2020 •
edited
Loading

It broke between these 2 builds from S3

last working build

Press any key to enter the REPL. Use CTRL-D to reload.
Adafruit CircuitPython 6.0.0-alpha.2-432-gfe73cfb92 on 2020-08-27; Adafruit CLUE nRF52840 Express with nRF52840
>>> 
>

not working


Press any key to enter the REPL. Use CTRL-D to reload.
Adafruit CircuitPython 6.0.0-alpha.2-434-g610002724 on 2020-08-27; Adafruit CLUE nRF52840 Express with nRF52840
>>> 
>>>

@jerryneedell
Copy link
Collaborator Author
jerryneedell commented Sep 1, 2020 •
edited
Loading

EDITED to ADD -- ignore this -- this was not the breaking commit -- see below

this commit made some changes to code common to all ports - no idea why to would impact CLUE and not PyPortal though ... a15f948

Nothing in the other commits jumps out as a likely cause.

@jerryneedell
Copy link
Collaborator Author
jerryneedell commented Sep 1, 2020 •
edited
Loading

hmm weird -- now

Press any key to enter the REPL. Use CTRL-D to reload.
Adafruit CircuitPython 6.0.0-alpha.2-432-gfe73cfb92 on 2020-08-27; Adafruit CLUE nRF52840 Express with nRF52840
>>> 
>``` 
 is not working any more ....

@jerryneedell
Copy link
Collaborator Author
jerryneedell commented Sep 1, 2020 •
edited 8000
Loading

Repeated the search and now find that the last working version was

Adafruit CircuitPython 6.0.0-alpha.2-26-gb7e6bf959 on 2020-07-25; Adafruit CLUE nRF52840 Express with nRF52840

and first broken version was

Adafruit CircuitPython 6.0.0-alpha.2-43-g7ab5c520e on 2020-07-25; Adafruit CLUE nRF52840 Express with nRF52840

HOWEVER when I reloaded 6.0.0-alpha.2-43-g7ab5c520e. it worked for REPL, but not when I ran a program that used the display. It did display but with odd behavior (extra lines in "spirit level") -- after that it no longer worked for REPL either.
so 6.0.0-alpha.2-43-g7ab5c520e appears to be causing problems that get worse...

6.0.0-alpha.2-26-gb7e6bf959 appears to be stable

This would implicate #3190 which chawed the compiler optimizations.

@jerryneedell
Copy link
Collaborator Author
jerryneedell commented Sep 1, 2020 •
edited
Loading

Reverting the changes in #3190 seems to fix it.

[Jerry-desktop-mini:circuitpython/ports/nrf] jerryneedell% git diff
diff --git a/ports/nrf/Makefile b/ports/nrf/Makefile
index 3fef68e88..1fdf79958 100755
--- a/ports/nrf/Makefile
+++ b/ports/nrf/Makefile
@@ -89,13 +89,11 @@ ifeq ($(DEBUG), 1)
   CFLAGS += -ggdb3
   OPTIMIZATION_FLAGS = -Og
 else
-  OPTIMIZATION_FLAGS ?= -O2
-  CFLAGS += -DNDEBUG -ggdb3
+  CFLAGS += -Os -DNDEBUG -ggdb3
   CFLAGS += -flto -flto-partition=none
 endif
 
 # option to override compiler optimization level, set in boards/$(BOARD)/mpconfigboard.mk
-CFLAGS += $(OPTIMIZATION_FLAGS)
 
 CFLAGS += $(INC) -Wall -Werror -std=gnu11 -nostdlib -fshort-enums $(BASE_CFLAGS) $(CFLAGS_MOD) $(COPT)

now works on the tip of main 6.0.0-alpha.3-62-gf0e60da51-dirty

Note: the above changes only reverted the "default" build changes in the Makefile -- there were also changes in the "debug' build.

I am not sure if there are other implications of changing this.

@jepler
Copy link
jepler commented Sep 1, 2020

A similar fix was offered at #3249 but closed without merge because @dhalbert filed #3281 and @DavePutz reported that it fixed #3247.

Were any other nrf-with-display boards tested? This reminds me, I had trouble with the Sharp Memory Display corrupting random lines of the display with nrf52840 and not on other micros. I chalked it up to the particular device being a Particle Xenon rather than an Adafruit board and moved on with a different board (that happened to be non-nRF)

@DavePutz
Copy link
Collaborator
DavePutz commented Sep 1, 2020

Issue #3296 is another case where compiling with -O2 optimization on the Clue broke something (in that case, BLE). @dhalbert has mentioned that these sorts of issues are most often due to a bug such as an uninitialized variable or something that should be volatile is not. I have spent a lot of time looking at #3296, and have traced it down to compiling ports/nrf/common-hal/busio/SPI.c with -O2 causing the BLE issue. But, I have not found anything in that source file that would seem to fit Dan's categories. Given the number of issues compiling with -O2 on the Clue seems to be causing, perhaps we should re-visit #3249?

@jerryneedell
Copy link
Collaborator Author
jerryneedell commented Sep 2, 2020 •
edited
Loading

this keeps getting stranger -- With the build I created yesterday - reverting the optimization settings, the CLUE works normally as long as it is connected to the computer USB port. If I power it via a battery alone, the Display does not work. The program in code.py runs, but nothing is displayed. If I go back to CP 5.3.1 it works normally via battery.... It seems like something in the display is requiring the USB connection to be active...
I'm confused!

@DavePutz
Copy link
Collaborator
DavePutz commented Sep 2, 2020 via email

@DavePutz
Copy link
Collab 8000 orator
DavePutz commented Sep 3, 2020 via email

@DavePutz
Copy link
Collaborator

@xiongyihui - what was your intent with the PR #3244 patch in regard to displays? Right now I see that qspi_enable() is being called during startup after qspi_disable(); but the Clue display does not come back on. I see that you did not use the NRF provided nrf_qspi_disable() to turn off QSPI, but you did use nrf_qspi_enable() to turn it back on. Was there a reason for this? I did find out the commenting out the
NRF_QSPI->TASKS_DEACTIVATE = 1;
line in qspi_disable() keeps the display on, but I don't know what impact that has when you want QSPI off. I' just trying to understand what's going on so that we can let the Clue display work on battery power.

@jerryneedell
Copy link
Collaborator Author
jerryneedell commented Sep 11, 2020 •
edited
Loading

This issue appears to have been resolved -- at least, it works on my CLUE now ... should this be closed?

NEVERMIND--- still not working on battery -- sorry for the confusion

@DavePutz
Copy link
Collaborator
DavePutz commented Sep 11, 2020 via email

@jerryneedell
Copy link
Collaborator Author

Sorry -- it is not working on battery -- I updated my comment above....

@DavePutz
Copy link
Collaborator

@jepler - do you know what the intent was for PR #3244? I asked @xiongyihui but have not received any response. Since qspi_disable() is called in port_sleep_until_interrupt(), any routine that calls mp_hal_delay_ms() will turn off QSPI. In effect, this disables QSPI by default at boot time when USB is not connected, as several startup routines have short delays in them. The effect on the Clue is to disable the display (which uses SPI) and there does not seem to be any way to turn it back on. Calling qspi_enable() does not turn the display back on. So, do we really want to have the Clue unable to use the display when on battery power?

@xiongyihui
Copy link

QSPI is not SPI. Normally, they don't affect each other. Does SPI share pins with QSPI?

@DavePutz
8000
Copy link
Collaborator
DavePutz commented Sep 16, 2020 via email

@tannewt
Copy link
Member
tannewt commented Sep 16, 2020

The intent of #3244 was to turn off the QSPI peripheral while going to sleep so that the high speed clock would be turned off and save a bunch of power. It should start back up at the next file access.

The SPI to the display should be separate from QSPI. Perhaps the SPI isn't keeping the high speed clock active?

@DavePutz
Copy link
Collaborator

I did some further testing, and it isn't only the display that's affected. An externally connected SPI device works with USB power but
does nothing with battery power. Logic analyzer shows no activity on the pins to the device at all. So, it looks like all SPI is affected when qspi is disabled.

@xiongyihui
Copy link

@DavePutz I tested it on nrf52840 m.2 devkit which has a SPI display. Turning off QSPI doesn't affect the SPI display.

image

@xiongyihui
Copy link

My test code is:


import time
import board
import digitalio

@micropython.asm_thumb
def mem(r0):
    ldr(r0, [r0, 0])

@micropython.asm_thumb
def mem_write(r0, r1):
    str(r1, [r0, 0])

peripherals = {
'RADIO': 0x40001000,
'UART0': 0x40002000,
'UARTE0': 0x40002000,
'SPI0': 0x40003000,
'SPIM0': 0x40003000,
'SPIS0': 0x40003000,
'TWI0': 0x40003000,
'TWIM0': 0x40003000,
'TWIS0': 0x40003000,
'SPI1': 0x40004000,
'SPIM1': 0x40004000,
'SPIS1': 0x40004000,
'TWI1': 0x40004000,
'TWIM1': 0x40004000,
'TWIS1': 0x40004000,
'NFCT': 0x40005000,
'GPIOTE': 0x40006000,
'SAADC': 0x40007000,
'TIMER0': 0x40008000,
'TIMER1': 0x40009000,
'TIMER2': 0x4000A000,
'RTC0': 0x4000B000,
'TEMP': 0x4000C000,
'RNG': 0x4000D000,
'ECB': 0x4000E000,
'AAR': 0x4000F000,
'CCM': 0x4000F000,
'WDT': 0x40010000,
'RTC1': 0x40011000,
'QDEC': 0x40012000,
'COMP': 0x40013000,
'LPCOMP': 0x40013000,
'EGU0': 0x40014000,
'SWI0': 0x40014000,
'EGU1': 0x40015000,
'SWI1': 0x40015000,
'EGU2': 0x40016000,
'SWI2': 0x40016000,
'EGU3': 0x40017000,
'SWI3': 0x40017000,
'EGU4': 0x40018000,
'SWI4': 0x40018000,
'EGU5': 0x40019000,
'SWI5': 0x40019000,
'TIMER3': 0x4001A000,
'TIMER4': 0x4001B000,
'PWM0': 0x4001C000,
'PDM': 0x4001D000,
'ACL': 0x4001E000,
'NVMC': 0x4001E000,
'PPI': 0x4001F000,
'MWU': 0x40020000,
'PWM1': 0x40021000,
'PWM2': 0x40022000,
'SPI2': 0x40023000,
'SPIM2': 0x40023000,
'SPIS2': 0x40023000,
'RTC2': 0x40024000,
'I2S': 0x40025000,
'FPU': 0x40026000,
'USBD': 0x40027000,
'UARTE1': 0x40028000,
'QSPI': 0x40029000,
'CC_HOST_RGF': 0x5002A000,
'CRYPTOCELL': 0x5002A000,
'PWM3': 0x4002D000,
'SPIM3': 0x4002F000
}

def show_peripherals():
    print('\nPeripherals:')
    for name, addr in peripherals.items():
        value = mem(addr + 0x500)
        if value:
            print('{}: {}'.format(name, value))


show_peripherals()


button = digitalio.DigitalInOut(board.USR_BTN)
button.direction = digitalio.Direction.INPUT
button.pull = digitalio.Pull.UP

last_value = button.value

while True:
    if last_value != button.value:
        last_value = button.value
        if last_value == 0:
            # print(last_value)
            show_peripherals()
    time.sleep(0.01)

@xiongyihui
Copy link
xiongyihui commented Sep 17, 2020 •
edited
Loading

On issue #3360, it seems turning off QSPI doesn't affect SPI too.

@tannewt
Copy link
Member
tannewt commented Sep 17, 2020

This seems very weird that USB vs battery would be different. Does the Clue wake up OK in this case? It seems like it must be related to the regulator: https://cdn-learn.adafruit.com/assets/assets/000/087/877/original/adafruit_products__CLUE_sch.png?1580423083 The only difference it will see is 5v from USB vs <4.2V from the battery.

@xiongyihui Is your schematic open source? I wonder how the power circuitry differs.

@xiongyihui
Copy link

The schematic is at https://github.com/makerdiary/nrf52840-m2-devkit

@DavePutz
Copy link
Collaborator
DavePutz commented Sep 18, 2020 via email

@tannewt
Copy link
Member
tannewt commented Sep 18, 2020

Thanks @xiongyihui! The power does look different on the feather as well.

https://cdn-learn.adafruit.com/assets/assets/000/068/545/original/circuitpython_nRF52840_Schematic_REV-D.png?1546364754

Looks like vbat goes through the diode of the transistor before the regulator.

@DavePutz do other things work ok? This is so weird that it works on USB but not battery power.

@DavePutz
Copy link
Collaborator

I determined that the calls to common_hal_time_delay_ms() in common_hal_displayio_fourwire_reset are the reason for this issue. Since common_hal_time_delay_ms() ends up calling port_sleep_until_interrupt(), which is where qspi_disable() is called; I suspect there must be some timing issue with disabling QSPI while the fourwire is being initialized. @tannewt , I see those delays were added in a commit for e-paper. If these delays are needed for e-paper to work, is there some other way to accomplish that? Maybe use mp_hal_delay_us()?

@tannewt
Copy link
Member
tannewt commented Sep 18, 2020

@DavePutz why would that only break when on battery power?

@DavePutz
Copy link
Collaborator

@tannewt - qspi_disable() only does the disabling when USB is not connected.

@cjsieh
Copy link
cjsieh commented Sep 18, 2020 via email

@xiongyihui
Copy link

@DavePutz
You can try to disable QSPI when USB is connected. Only need to change the line at https://github.com/adafruit/circuitpython/pull/3244/files#diff-46ca91d54ad3cf6fcc4906eecc666ebaR45 to

    if (NRF_QSPI->ENABLE) {

I tried it. The SPI display still works when QSPI is off and USB is connected.

@DavePutz
Copy link
Collaborator

OK, here is the issue the way I see it. If the two calls to common_hal_time_delay_ms(1) in common_hal_displayio_fourwire_reset() are replaced with mp_hal_delay_us(1000) then the SPI TFT displays on Clue and Feather work fine. However, an e-paper display on the Feather still does not work, due to the call to common_hal_time_delay_ms() in send_command_sequence(). The delay there looks like it might be for quite a long time (up to 500ms). Assuming that the e-paper displays need that delay, I can think of two alternatives. Either send_command_sequence() gets modified in some way to not call common_hal_time_delay_ms(); or we need to take the qspi_disable() call out of port_sleep_until_interrupt() (possibly by making it a call of its own, so it is a deliberate choice and not the default?) @tannewt , @xiongyihui - any thoughts?

@tannewt
Copy link
Member
tannewt commented Sep 21, 2020

Is the sleep too short when qspi is disabled? I still don't understand the connection between disabling QSPI and SPI display. Maybe there is a clock glitch on the SPI lines when sleeping?

@DavePutz
Copy link
Collaborator
DavePutz commented Sep 21, 2020 via email

@xiongyihui
Copy link

common_hal_time_delay_ms will delay uncertain time because of background tasks.
If initializing these display is time-critical, mp_hal_delay_us is a better option.

@tannewt
Copy link
Member
tannewt commented Sep 22, 2020

@xiongyihui I doubt those delays are broken if it takes longer than expected. Is there some way for the delay to shorten with the qspi disable?

@DavePutz
Copy link
Collaborator
DavePutz commented Sep 22, 2020 via email

@xiongyihui
Copy link

I think qspi_disable() takes less than 10 us. The uncertain delay is caused by background tasks, not by qspi_disable() .

@DavePutz
Copy link
Collaborator

It seems as though the fix for nRF52840 Revision 1 Errata V1.4 - 3.8 in qspi_disable() is doing something to the SPI. If I comment out the two lines from that errata, SPI works correctly. Then running on battery I do not see QSPI in the peripheral list generated by the script above from @xiongyihui . However, I don't have a way of measuring any differences in the current draw from the battery with or without that code. I have not been able to find any reference saying what the line
*(volatile uint32_t *)0x40029054 = 1;
actually does.

@tannewt
Copy link
Member
tannewt commented Sep 25, 2020 •
edited
Loading

@DavePutz Thank you for finding that! That's enough root cause for me.

@tannewt tannewt linked a pull request Sep 28, 2020 that will close this issue
Issue #3367 Changed delay calls to avoid conflicts with qspi_disable #3453
Merged
tannewt added a commit that referenced this issue Sep 28, 2020
@tannewt
Merge pull request #3453 from DavePutz/issue3367
ffeaef5 
Issue #3367 Changed delay calls to avoid conflicts with qspi_disable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug displayio
Projects
None yet
Milestone
6.0.0
Development

Successfully merging a pull request may close this issue.

Issue #3367 Changed delay calls to avoid conflicts with qspi_disable
6 participants
@tannewt @xiongyihui @jepler @jerryneedell @cjsieh @DavePutz
0