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

Fix for issue #3383 - SAMD5x microcontroller.cpu.voltage #4862

Merged
merged 3 commits into from
Jun 9, 2021
Merged

Fix for issue #3383 - SAMD5x microcontroller.cpu.voltage #4862

merged 3 commits into from
Jun 9, 2021

Conversation

DavePutz
Copy link
Collaborator

@DavePutz DavePutz commented Jun 5, 2021

Issue #3383. Found an answer online; we need to clear the VREF_ONDEMAND bit before reading SAMD5x voltage. Tested OK on an Itsy-Bitsy M4 Express(SAMD51); I don't have an SAMD54 to test with.

@dhalbert
Copy link
Collaborator

dhalbert commented Jun 5, 2021

I will test this on Monday or Tuesday with a SAMD51 board that has this problem. Could you point to the solution you found?

@dhalbert dhalbert self-requested a review June 5, 2021 22:06
@dhalbert dhalbert self-assigned this Jun 5, 2021
@DavePutz
Copy link
Collaborator Author

DavePutz commented Jun 5, 2021

I found the issue mentioned and answered at https://community.atmel.com/forum/samd51-using-intref-adc-voltage-reference

@dhalbert
Copy link
Collaborator

dhalbert commented Jun 6, 2021

https://community.atmel.com/forum/samd51-using-intref-adc-voltage-reference is a thread I started (username "danhalbert"), and reported on some experimentation I was doing. I think that solution ended up not working consistently, but I'll retry it to find out.

@DavePutz
Copy link
Collaborator Author

DavePutz commented Jun 6, 2021

@dhalbert , I apologize for not noticing your login! All I can report is that my Itsy-Bitsy M4 Express was hanging before I put the patch in and is now working with the patch.

@dhalbert
Copy link
Collaborator

dhalbert commented Jun 7, 2021

I have a Metro M4 that was exhibiting the voltage hang in 5.3.1. It does not have that problem in 6.3.0, and it also works with your patch. This is the diff of the relevant code between 5.3.1 and 6.3.0:

-#ifdef SAMD51
+#ifdef SAM_D5X_E5X
     hri_supc_set_VREF_SEL_bf(SUPC, SUPC_VREF_SEL_1V0_Val);
-    // ONDEMAND must be clear, and VREFOE must be set, or else the ADC conversion will not complete.
-    // See https://community.atmel.com/forum/samd51-using-intref-adc-voltage-reference
-    hri_supc_clear_VREF_ONDEMAND_bit(SUPC);
     hri_supc_set_VREF_VREFOE_bit(SUPC);
+
     adc_sync_set_reference(&adc, ADC_REFCTRL_REFSEL_INTREF_Val);
+
+    // On some processor samples, the ADC will hang trying to read the voltage. A simple
+    // delay after setting the SUPC bits seems to fix things. This appears to be due to VREFOE
+    // startup time. There is no synchronization bit to check.
+    // See https://community.atmel.com/forum/samd51-using-intref-adc-voltage-reference
+    mp_hal_delay_ms(1);

What happened originally is that @jepler noticed a problem on his SAME54 dev board. I added the delay, which helped on some boards but not all. Once the delay was in place, the ONDEMAND did not seem to make a difference (or maybe it made things worse?) so I removed it.

Currently microcontroller.cpu.voltage is disabled on SAME54? Could you re-enable it by removing this code?

#ifndef MICROCONTROLLER_VOLTAGE_DISABLE
#define MICROCONTROLLER_VOLTAGE_DISABLE (1)
#endif
. Then push that change.

Then, @jepler, could you test the artifact on your SAME54 Xplained board? I tried .voltage on my own SAME54 Xplained sample, but it works fine with or without this change. Thanks both!

@DavePutz
Copy link
Collaborator Author

DavePutz commented Jun 8, 2021

@dhalbert , @jepler ; I pushed the change to enable microcontroller.cpu.voltage on SAME54. Also did some more testing on a different SAMD51 board. If I take out the call to mp_hal_delay_ms it hangs even with this patch. If I take out the patch it hangs even with the delay. So - maybe we need both?

@dhalbert
Copy link
Collaborator

dhalbert commented Jun 8, 2021

If I take out the call to mp_hal_delay_ms it hangs even with this patch. If I take out the patch it hangs even with the delay. So - maybe we need both?

Yes, let's have both. I think there may be two problems in the internal circuitry and we have to compensate for both.

@jepler
Copy link
Member

jepler commented Jun 9, 2021

Hi! Sorry for the delay in replying.

First off, it's important to note that sometimes reading the CPU voltage alone is fine. On the sam e54 board I have, the problem occurs after reading temperature:

>>> import microcontroller
>>> microcontroller.cpu.voltage
3.33089
>>> microcontroller.cpu.temperature
27.201
>>> microcontroller.cpu.voltage
[hangs forever]

(this is "7.0.0-alpha.3-29-gc6dee5288-dirty" on a same54_xplained but with VOLTAGE_DISABLE re-enabled)

.. recommendation: the testing procedure for reading microcontroller.cpu.voltage should include also reading temperature and possibly other AnalogIn activities.

The good news is, that with the board that failed above, it can now read CPU voltage, including after reading temperature or an arbitrary analog input:

>>> a = analogio.AnalogIn(board.PA04)
>>> a.value
15904
>>> microcontroller.cpu.voltage
3.32503
>>> microcontroller.cpu.voltage
3.32698
>>> microcontroller.cpu.temperature
28.4751
>>> microcontroller.cpu.voltage
3.32894

Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

Testing on a SAM E54 Xplained worked (yay!) but some small improvements can be made to the PR.

@dhalbert
Copy link
Collaborator

dhalbert commented Jun 9, 2021

some small improvements can be made to the PR

I didn't see your suggestions for this.

@dhalbert
Copy link
Collaborator

dhalbert commented Jun 9, 2021

I retested my bad board as well, and it will read voltage after reading temperature without hanging.

ports/atmel-samd/mpconfigport.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for revisiting this.

@dhalbert dhalbert requested a review from jepler June 9, 2021 19:01
Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes

@jepler jepler merged commit 26a75f7 into adafruit:main Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants