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.

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

Occasional timeout on readADC() when running on system with RTOS such as ESP6288 #82

Closed
deKees687 opened this issue Oct 17, 2024 · 27 comments
Closed

Occasional timeout on readADC() when running on system with RTOS such as ESP6288 #82

deKees687 opened this issue Oct 17, 2024 · 27 comments
Assignees
@RobTillaart
Labels
enhancement New feature or request

Comments

@deKees687
Copy link

The readADC() functions uses a very short timeout of only a few milliseconds more than the actual conversion time. The yield() function may cause a context switch resulting in a delay for several milliseconds. As a result, the readADC() reports a timeout.

See https://www.circuitsonline.net/forum/view/167087

Can be solved by postponing the yield until after the timeout-check.
And by allowing a minimum timeout of 10 milliseconds.
@RobTillaart RobTillaart self-assigned this Oct 17, 2024
@RobTillaart RobTillaart added the enhancement New feature or request label Oct 17, 2024
@RobTillaart
Copy link
Owner

Thanks for reporting this issue and proposing a solution.

Will look into it asap, might be a few days.

@deKees687
Copy link
Author

I have sent a pull request with the fix yesterday (my first pull request ever) but it seems to have disappeared.

@RobTillaart
Copy link
Owner

Strange, please try it again.

@deKees687
Copy link
Author

deKees687 commented Oct 17, 2024 via email

@RobTillaart
Copy link
Owner

Read once that e.g. the "networking threads" stay happy if it can check about every 5 ms.

Propose to place the call to yield() both before and after the timeout test.

Something like

int16_t ADS1X15::_readADC(uint16_t readmode)
{
  _requestADC(readmode);
  if (_mode == ADS1X15_MODE_SINGLE)
  {
    yield();   //  yield for RTOS e.g. ESP.
    uint32_t start = millis();
    //  timeout == { 129, 65, 33, 17, 9, 5, 3, 2 }
    //  a few ms more than max conversion time.
    uint8_t timeOut = (128 >> (_datarate >> 5)) + 1;
    while (isBusy())
    {
      if ( (millis() - start) > timeOut)
      {
        return ADS1X15_ERROR_TIMEOUT;
      }
    }
    yield();
  }
  else
  {
    //  needed in continuous mode too, otherwise one get old value.
    delay(_conversionDelay);
  }
  return getValue();
}

@deKees687
Copy link
Author

deKees687 commented Oct 17, 2024 via email

@RobTillaart
Copy link
Owner

The difference is that I moved yield() completely out of the loop.
Which on second sight could cause my code to be blocking way too long.

Need more time to think over the consequences.

Can be solved by postponing the yield until after the timeout-check.
And by allowing a minimum timeout of 10 milliseconds.

Adding the 10 ms is quite a lot for certain data rates as the timeout (specs) is data rate dependent.
However as timeouts do not happen often this seems acceptable.

@deKees687
Copy link
Author

deKees687 commented Oct 17, 2024 via email

@RobTillaart
Copy link
Owner

I would not do that.

Agree,
(thinking out loud) however if one adds 10 ms to the timeout is it really needed to move the yield() after the timeout check?
this check takes a few micros so before or after should not make a big difference.

@RobTillaart
Copy link
Owner

Also note that not all RTOS versions depend on the yield() function to switch context. Most systems do context switching on timer interrupt, so you have no control over when they may occur. So for these cases you need a longer timeout value. I would suggest a 10 mS minimum.

OK, the 10 ms will be added, as it will only delay a failing conversion so.

I will make a develop branch and bump the version number for it.

@deKees687
Copy link
Author

deKees687 commented Oct 17, 2024 via email

@deKees687
Copy link
Author

deKees687 commented Oct 17, 2024 via email

RobTillaart added a commit that referenced this issue Oct 17, 2024
@RobTillaart
fix #82, minimal timeout 10 ms for RTOS
4ff8350
@RobTillaart
Copy link
Owner

Develop branch and PR created.

@RobTillaart
Copy link
Owner

Yes it does make a big difference:

Imagine:

Is busy() Yes
yield() Context switch in yield takes 10 ms
Is timeout? Yes -> error

Or

Is busy() Yes
Is timeout? No not yet -> continue
yield() Context switch in yield takes 10 ms
Is Busy? No -> readValue

Not agree,

If the context switch is timer based (as you state also occurs) the switch can still be between the isBusy() check and the timeout check. So moving the yield() call after the timeout check does not solve this and scenario 1 can still occur.

The 10 ms extra will catch this.

In case of a good conversion,

  • the isBusy() returns true and the duration is below the timeout as specified in the datasheet for actual data rate.
  • Assume context switching the timeout check is up to 10 ms later.
  • Then the timeout check has the10 ms extra margin to properly detect the timeout or not.

That said, moving the yield() is for yield() based context switching better so it was moved.

@RobTillaart
Copy link
Owner

@deKees687

Can you give the develop branch a try?
If OK I will merge it into master and release version 0.5.1

(now quickly catch up my other work ;)

@RobTillaart
Copy link
Owner

RobTillaart commented Oct 17, 2024 •
edited
Loading

Read the https://www.circuitsonline.net/forum/view/167087 thread.

It also mentioned that the library returning an error code instead of a value was not smart.

      if ( (millis() - start) > timeOut)
      {
        return ADS1X15_ERROR_TIMEOUT;
      }

Will look into that too.

  • At least the error flag should be set too.

RobTillaart added a commit that referenced this issue Oct 17, 2024
@RobTillaart
fix #82, minimal timeout 10 ms for RTOS
775ac76
@deKees687
Copy link
Author

deKees687 commented Oct 17, 2024 via email

@deKees687
Copy link
Author

deKees687 commented Oct 17, 2024 via email

@RobTillaart
Copy link
Owner

error

Question is,

If timeout what value to return as any value >= 0 incl last valid value is as incorrect as the current -101 constant.

Not changing the -101 is at least backwards compatible.

Examples should indeed be updated (or at least some)
Documentation must be extended.

@deKees687
Copy link
Author

deKees687 commented Oct 17, 2024 via email

@deKees687
Copy link
Author

deKees687 commented Oct 17, 2024 via email

@RobTillaart
Copy link
Owner

That would be a better interface. Breaking but better, bool readADC(int channel, int &value)
Would need to patch more functions in the API to apply this same pattern.
And update and test all examples.
Will be a major update so I'll make a separate issue for that.

@RobTillaart RobTillaart mentioned this issue Oct 17, 2024
Open
@RobTillaart
Copy link
Owner

Created issue for API - #84

@deKees687
Copy link
Author

deKees687 commented Oct 17, 2024 via email

@RobTillaart
Copy link
Owner

You may even consider a nonblocking interface.

Is already available, from readme.md

To read the ADC in an asynchronous way (e.g. to minimize blocking) you need call three functions:

  • void requestADC(uint8_t pin = 0) Start the conversion. pin = 0..3.
    Default pin = 0 as this is convenient for 1 channel devices.
  • bool isBusy() Is the conversion not ready yet? Works only in SINGLE mode!
  • bool isReady() Is the conversion ready? Works only in SINGLE mode! (= wrapper around isBusy() )
  • int16_t getValue() Read the result of the conversion.

in terms of code:

  void setup()
  {
    //  other setup things here
    ADS.setMode(1);               //  SINGLE SHOT MODE
    ADS.requestADC(pin);
  }

  void loop()
  {
    if (ADS.isReady())
    {
      value = ADS.getValue();
      ADS.requestADC(pin);       //  request new conversion
    }
    //  do other things here
  }

@deKees687
Copy link
Author

deKees687 commented Oct 17, 2024 via email

RobTillaart added a commit that referenced this issue Oct 17, 2024
@RobTillaart
fix #82, minimal timeout 10 ms for RTOS
11755d1
RobTillaart added a commit that referenced this issue Oct 17, 2024
@RobTillaart
fix #82, minimal timeout 10 ms for RTOS
6cbd9f9
@RobTillaart
Copy link
Owner

@deKees687

Discussions about API can continue in #84

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@RobTillaart RobTillaart

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@RobTillaart @deKees687