SpenceKonde / megaTinyCore

Arduino core for the tinyAVR 0/1/2-series - Ones's digit 2,4,5,7 (pincount, 8,14,20,24), tens digit 0, 1, or 2 (featureset), preceded by flash in kb. Library maintainers: porting help available!
Other
551 stars 143 forks source link

analogRead after disabling the adc and sleeping does not work #834

Closed markusschweitzer closed 1 year ago

markusschweitzer commented 1 year ago

Hi, on the Attiny3226 and version 2.6.1, I try to disable the adc, sleep in power down mode and enable the adc again, and read a value again:

  pinMode(3, INPUT);
  int val = analogRead(3);
  Serial.println(val);

  ADCPowerOptions(ADC_DISABLE);
  sleep_cpu();
  ADCPowerOptions(ADC_ENABLE);

  Serial.println("awake!");

  int valNew = analogRead(3);
  Serial.println(valNew);

Before sleeping and disabling the adc, the value was read correctly. After the sleep (after "awake" was printed), it hangs at analogRead() - the call never finished (waited minutes...)

Did I miss anything here?

Thanks in advance

SpenceKonde commented 1 year ago

Did I miss anything here?

Probably not. It's not like I actually tested that addition or anything, beond confirming that the CI still ran. Run this and report back what it prints in the console.

  // it is assumed that at some point you started serial running ar a reasonable speed. 
  pinMode(3, INPUT); // Bad practice warning: don't use numbers to refer to pins, it has no upside potential and non-zero downside. Use the provided names, ex, for pin 3, on a 3226, that's pin PA7 so whenever you reference it d
  int val = analogRead(3);
  Serial.println(val);
  uint8_t * t;
  t=(uint8_t *) &ADC0;
  Serial.printHex(t, 0x0D, ':'); // print out all the configuration registers of the ADC..

  ADCPowerOptions(ADC_DISABLE);
  t=(uint8_t *) &ADC0;
  Serial.printHex(t, 0x0D, ':'); // make sure we got the disable right
  Serial.flush()
  sleep_cpu();
  Serial.println("awake!");
  t=(uint8_t *) &ADC0;
  Serial.printHex(t, 0x0D, ':'); // See how it looks on wake
  ADCPowerOptions(ADC_ENABLE);
  t=(uint8_t *) &ADC0;
  Serial.printHex(t, 0x0D, ':'); // See how it looks on reenabling
  int valNew = analogRead(3);
  Serial.println(valNew);
  Serial.flush();// make sure that evertyhing done printing before whatever comes next. 

That will add 4 blocks of 14 hexadecimal numbers (that's the entirety of the config for the ADC), each byte separated by a colon, at 4 key points in the process. Presumably at the end, it's not in the same state as it was when it started, and this will tell use exactly what the difference is. , except for the windowed comparator mode stuff that I don''t think gets much use. and which we can be confident isnt' relevant here.

This is really the problem with CI tests - it tests that things compile,. but we have no idea of whether the behavior is right.

If anyone wants to write a script which would run on a raspberry pi off in a corner somewhere on a daily cronjob..which would sync the repo to the hardware subdir of sketchbook, compile and upload a sketch,. then open another serial port to read output from the sketch and log it. Grep file for "FAIL", rename the log file to (todays date)_ATTiny3227_PASS.log, and then use scp to upload it to /var/www/html/results on azduino.com (if anyone did this, I would be overjoyed, and would and would be more than happy to set up the credentials to actually let it log in.), that would be an incredible service to the community.

I'd still need to write a decent test script for each part, bit it wouldn't be hard to write a sketch that checks all the places I thought were likely to harbor potentially brittle code, but since I wouldn't have a way to makeit run automatically r, all we get is compile-tests. I'd probably try to rig it up to test on a tinyAVR 3217, a 3227, and an AVR128D64 and AVR64DD32. Assuming a dedicated serial adapter for each board's UPDI and Serial ports, that's still only 8USB ports, so you could handle it on a single usb hub. I would consider it acceptable to assume that arduino is installed, and that it's had its's "stock" toolchain "hacked" to be equivalent to the latest Azduino toolchain (since It would only need to be done a couple of times a year, that's best handled by wetware imo.. Being able to know within a day if a submitted change broke anything within a subset of behavior that was tested on ACTUAL HARDWARE would be incredibly useful.

We all have figured out that all the code simulators are largely useless, as their behavior dosn't match the hardware. so it's gotta be on actual hardware. Since I'd be writing the test code and building the hardware, I could do dirty things like connecting pins physically so the chip can say use USART1 to print some text, and receive it on USART0 generate a signal that it then treats like an external one), and nobody knows better than I do where all the bodies are buried in the cores.

The results would be publicly visible that's a public directory with directory listing enabled) so anyone could investigate exactly when something started going awry and yell at the person (probably me) responsible.

Because the biggest problems with the cores isthat things get broken and nobody notices until weeks after a release.

markusschweitzer commented 1 year ago

It still ahngs but the reason for this seems to be visible now:

0
21:00:28:00:00:02:00:00:0F:00:10:0A:07
21:00:28:00:00:02:00:00:0F:00:10:0A:07
awake!
21:00:28:00:00:02:00:00:0F:00:10:0A:07
20:00:28:00:00:02:00:00:0F:00:10:0A:07

After the disable nothing change? (This cant be right, or?) And after the enable, the first bit of the first byte is not set.

Also, I debugged it a little bit and figured that it is this whats blocking: while (!(ADC0.INTFLAGS & ADC_RESRDY_bm)); and there especially the ADC_RESRDY_bm.

Since Im not really sure how the right way to fix this is - Im just poking around with a stick and see what changes - I hope this helps at least a little bit.

btw: would this be a very crude way of turning off the adc and back on again after the sleep?

  uint8_t adc = ADC0.CTRLA;
  ADC0.CTRLA = 0;
  sleep_cpu();
  ADC0.CTRLA = adc;

best regards

SpenceKonde commented 1 year ago

I'm going to guess that the enable/disable logic is straight up backwards....

Your method should work.

Note that you should check with a precision current meter (if you have one) and see if you get a difference in sleep current between:

  uint8_t adc = ADC0.CTRLA;
  ADC0.CTRLA = 0;
  sleep_cpu();
  ADC0.CTRLA = adc;

and

  uint8_t adc = ADC0.CTRLA;
  ADC0.CTRLA = 0x20;
  sleep_cpu();
  ADC0.CTRLA = adc;

Let me know if you do this and what your results are. I'm not well set up to test this, and I suspect the results (which should not be different) in some cases,in fact are be different. Seeing as that seems to be about the only area of the chip they screwed up (compare to the tinyAVR 1-series parts, for example, where the list of errata (not their descriptions, just the list) goes on for like two pages and the DA with most of the same errata, plus a datasheet "clarification" that erased a zero from the rewrite endurance - after the product had been shipping for 2 years) I'd say they did pretty well.

(the difference between errata and datasheet clarifications like that is that they pretend that errata will be get fixed. In rare cases, they actually DO get fixed. A datasheet clarification on the other hand is them saying "Yeah, we're not even going to pretend that we'll fix it". For example, on the early ATtiny3216's, you couldn't turn on the RTC without also turning on the pit, if you wanted things to work right, but the ones you get now fixed that. However, that is one of very few cases where they've actually fixed modern AVR bugs with a die rev. Usually updates to the silicon errata are to add newly discovered bugs, not announce a new die rev that fixes things.).

0x20 is the lowlat bit. Unfortunately it's behavior is rather funky, and i haven't conducted a study into exactly how it actually behaves and whether the behavior is even the same across the 2-series. The errata is not listed for all the 2-series parts, but one of the ones that doesn't list it is the 16k ones, and they came out first, implying that they added a new bug to an ADC with no outstanding issues (what the hell they were mucking around with it for just to add more memory. More likely, I think is that all 2-series have this suite of ADC power consumption issues and Microchip doesn't update errata as often as they should.

SpenceKonde commented 1 year ago

Any update?

markusschweitzer commented 1 year ago

It seems to work for now. Since my multimeter is now showing 0 uA I guess it works - I will get a more precise one later next week. I guess this can be closed. Thanks for your help.