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

Adding functionality and ESP32-S3 support #5

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

Mirageofmage
Copy link

This PR adds functionality for the Gain set function of the ADC, as well as fixes an issue that prevents the library from working on the ESP32-S3 modules

@nerdyscout
Copy link
Owner

nerdyscout commented Dec 9, 2022

I merged those changes to the develop branch. feel free to test.

Copy link
Author

@Mirageofmage Mirageofmage left a comment

Choose a reason for hiding this comment

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

Some of these changes are because I had an out of date copy of the file locally. I.e. all of the URLs, the interrupt functionality, and the debug flags.

@nerdyscout
Copy link
Owner

  • could you please confirm to removeable of no/interrupts()? This seems wrong to me, I had some serious issues here!
  • I would prefer to have seperate commits for each things clearly documented. It is also hard for me to stick to this....
  • I am currently working on some changes which will break you latest contribution, therefore currently will not merge anything further.

nerdyscout and others added 17 commits October 10, 2023 10:35
* [pre-commit.ci] pre-commit autoupdate

updates:
- [github.com/pre-commit/pre-commit-hooks: v4.3.0 → v4.5.0](pre-commit/pre-commit-hooks@v4.3.0...v4.5.0)

* Update .pre-commit-config.yaml

add clang-format for pre-commit-hook

* Update .pre-commit-config.yaml

fix spaces

* Update .pre-commit-config.yaml

rev is mandatory

* Update .pre-commit-config.yaml

use explicit version

* Update .pre-commit-config.yaml

* Update .pre-commit-config.yaml

* Update .pre-commit-config.yaml

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update .pre-commit-config.yaml

* Update .pre-commit-config.yaml

* Update .pre-commit-config.yaml

* Update .pre-commit-config.yaml

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update .pre-commit-config.yaml

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: nerdyscout <nerdyscout@users.noreply.github.com>
refactored to make Settings() a class within which will hold all configuration register values
@LeKrakenTheCode
Copy link

Hey, I am trying to get this library to work with an ESP32-S3-mini, which should be the same as just the S3. It keeps freezing when the analogRead(MCP_CH0) is called. I would be happy to give more info if needed. Is this the same issue as mentioned above and if so can I help troubleshoot?

@nerdyscout
Copy link
Owner

Hi @LeKrakenTheCode!
I would be really happy to close this issue, so every info is highly welcomed.
do you run main or develop?

@LeKrakenTheCode
Copy link

LeKrakenTheCode commented Dec 27, 2023

I am running main currently. I have made the changes to the struct as well as added the arguments to the begin statement on line 96 of the cpp file. That has fixed my "freeze" (which now i realize was the endless loop mentioned above), however I am still not getting any values other than zero with the analogread and conversion. Thanks in advance, your library is super well written. Should I move to the develop branch?

@Mirageofmage
Copy link
Author

Mirageofmage commented Dec 27, 2023

I am running main currently. I have made the changes to the struct as well as added the arguments to the begin statement on line 96 of the cpp file. That has fixed my "freeze" (which now i realize was the endless loop mentioned above), however I am still not getting any values other than zero with the analogread and conversion. Thanks in advance, your library is super well written. Should I move to the develop branch?

Did you also update the union in the header file? Check the updates in this commit

The develop branch has all of the changes made in this PR and should work on the S3

@LeKrakenTheCode
Copy link

LeKrakenTheCode commented Dec 27, 2023

I am running main currently. I have made the changes to the struct as well as added the arguments to the begin statement on line 96 of the cpp file. That has fixed my "freeze" (which now i realize was the endless loop mentioned above), however I am still not getting any values other than zero with the analogread and conversion. Thanks in advance, your library is super well written. Should I move to the develop branch?

Did you also update the union in the header file? Check the updates in this commit

I have updated the union (i said struct by mistake). I added _status = read(&adcdata); to the analogRead function after the while loop as well. As I am using the MCP3462T-E/NC I also changed the reference to 2.4 from 3.3 if that matters.
I found one mistake i have made, I meant to use the MCP3462R which has an internal reference but i used the T. So right now the ADC has nothing connected to the reference pins which is probably why i cannot get a voltage read.

@nerdyscout
Copy link
Owner

nerdyscout commented Dec 27, 2023 via email

@LeKrakenTheCode
Copy link

LeKrakenTheCode commented Jan 2, 2024 via email

@LeKrakenTheCode
Copy link

I got in the correct chip now with internal reference. I tried with the changes I made to the main branch code, but still am not getting any voltage back. I moved to the develop branch and I had to update the union again along with the spi->begin arguments. Back to the same issue where i simply don't get any value other than 0 from the analogRead.

Hope you had a good holiday!. I have done more testing and can confirm I have the pins correct as if one of them is not correct it doesn't finish the analog read. So I have been checking the config registers next and still can't find any problems. Can you think of any reasons I would only be getting an analogRead of 0? The reference is now enabled in the correct chip. (according to the registers)

@Mirageofmage
Copy link
Author

Mirageofmage commented Jan 23, 2024 via email

@LeKrakenTheCode
Copy link

image
image
I have tried both reading channel 1 using MUX_CH1 and the built in temperature. Currently it is this:
image

@Mirageofmage
Copy link
Author

image image I have tried both reading channel 1 using MUX_CH1 and the built in temperature. Currently it is this: image

Did you call adc.begin() in setup?

@LeKrakenTheCode
Copy link

image image I have tried both reading channel 1 using MUX_CH1 and the built in temperature. Currently it is this: image

Did you call adc.begin() in setup?

yes, forgot to include that:
image
image

@nerdyscout
Copy link
Owner

  • REFin- should be connected to GNDA
  • GNDD and GNDA should be somwhere connected
  • atleast on develop you should use adc.begin(0), not setting Vref! begin() will internally call setReference(vref) which enables internal reference as vref defaults to 0
void MCP3x6x::setReference(float vref) {
  if (vref == 0.0) {
    vref                      = 2.4;
    settings.config0.vref_sel = 1;
    _status                   = write(settings.config0);
  }
  _reference = vref;
}

anyway... none of these really explains getting zero values, I would expect noise but not zeros.

@LeKrakenTheCode
Copy link

LeKrakenTheCode commented Jan 23, 2024 via email

@Mirageofmage
Copy link
Author

@.*** I do have AGND and GND connected via a ferrite. Good catch with the AGND on REFIN-, Thank you. I was also expecting some noise as well. Especially at 16 bit.

Your REF+ is floating? On my design I have that connected to 3V3

@nerdyscout
Copy link
Owner

Are you measuring in single ended or differential mode?

and Mux mode or continuous mode?

@LeKrakenTheCode
Copy link

LeKrakenTheCode commented Jan 23, 2024 via email

@LeKrakenTheCode
Copy link

LeKrakenTheCode commented Jan 23, 2024 via email

@nerdyscout
Copy link
Owner

yes MuxMode should be fine by default, but I never used it why I am lacking experience.

int32_t MCP3x6x::analogRead(mux_t ch) {
  // MuxMode
  if (settings.scan.channel.raw == 0) {
#ifdef MCP3x6x_DEBUG
    Serial.println("mux");
#endif
    settings.mux = ch;
    _status      = write(settings.mux);
    _status      = conversion();
    while (!_status.dr) {
      _status = read(&adcdata);
    }
    return result.raw[(uint8_t)adcdata.channelid] = adcdata.value;
  }

freezing in here should only be possible if the while() is never returned. best would be if you could connect a debugger and step through to see where the read() fails.

could you please double check if you got this ":1" which @Mirageofmage fixed ? this is essential and would explain freezes.

  typedef union __attribute__((__packed__)) {
    struct {
      struct {
        bool por:1;     //!< status: power on reset
        bool crccfg:1;  //!< status: crc
        bool dr:1;      //!< status: data ready
      };
      uint8_t      : 1;  //!< !addr[0]
      uint8_t addr : 2;  //!< addresse
      uint8_t      : 2;  //!< EMTPY
    };
    uint8_t raw;
  } status_t;
  status_t _status;

@LeKrakenTheCode
Copy link

LeKrakenTheCode commented Jan 23, 2024 via email

@nerdyscout
Copy link
Owner

nerdyscout commented Jan 23, 2024

\\ line 273
  if (settings.scan.channel.raw == 0) {
#ifdef MCP3x6x_DEBUG
    Serial.println("mux");
#endif
    settings.mux = ch;
    _status      = write(settings.mux);
    _status      = conversion();
    while (!_status.dr) {
      _status = read(&adcdata);
    }
    return result.raw[(uint8_t)adcdata.channelid] = adcdata.value;
  }

just another wild guess:
try removing _status = on conversion() and write(settings.mux) maybe even but the conversion() into the loop

@LeKrakenTheCode
Copy link

LeKrakenTheCode commented Jan 23, 2024 via email

@Mirageofmage
Copy link
Author

What's the speed of your SPI bus? I remember having some issues if the speed was over 500k. I'm currently running w/ 200k

@Mirageofmage
Copy link
Author

Also, @LeKrakenTheCode should probably open a new issue while this can be pulled into branch

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.

3 participants