Skip to content

Added README, example and cache feature #2

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

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

Conversation

Rocco83
Copy link
Contributor

@Rocco83 Rocco83 commented May 8, 2021

The README is a stripped down version of the wiki page, just to start
The example is taken from the wiki page as well
The cache functionality add a feature to allow digitalRead to use the in memory data, saved from a single digitalRead with cache = false or any portRead.

Rocco83 added 5 commits May 5, 2021 18:20
The CSDataArray will change over the time, must be fixed in advance
Added # with '#' will be ignored, and an empty message aborts the commit.
@macegr
Copy link
Member

macegr commented May 21, 2021

Not merging this for the moment:

  • Don't want to change the function signature for digitalRead(), breaking existing sketches. The cache option could be added as a function overload instead, so that the cache could be used optionally without breaking anything else.
  • Cache function (reason for using it etc) not well explained. Normal Arduino design style is to have digitalRead() actually go read the hardware, which is what the Centipede does. portRead() can be used to get a full port's width of data, and the user could cache this if they wanted to break with Arduino-style behavior. As mentioned above, the cache function could stay in if it were overloaded, but not 100% sure as it changes the meaning of the digitalRead() functionality. Perhaps breaking it out entirely to a cachedDigitalRead() function would make it more explicit to the user.
  • Several typos in comments and docs
  • Commits that revise previous code in the same PR should be squashed, we can do this during merge but the history within the PR itself remains harder to follow.

@Rocco83
Copy link
Contributor Author

Rocco83 commented May 22, 2021

Thanks for the comment,
Take mine from a newbie perspective :-)
And thanks for taking the time to review it.

Let me explain my use case of cache.
I'm using CS (still writing the code) paired with Bounce2 library.
I have already overloaded Bounce2, but Bounce2 does not have -- afaik -- a port concept.
Bounce2 works calling .update method in the loop "per pin", so potentially 16 portRead instead of 1 (as digitalRead is a wrapper of portRead).

With cache, i call in advance portRead, to all the ports.
Then I manage to call Bounce2 per pin, which will not even interlock with MCP, it will just take the current value.

In a nutshell (and feel free to spot errors, if you see it):

class BounceCS : public Bounce {
  public:
     bool readCurrentState() {
      return CS.digitalRead(pin, false);
    }
    void setPinMode(int pin, int mode) {
      CS.pinMode(pin, mode);
    }
};                                                                                                
#define NUM_BUTTONS 64
BounceCS * inputStatus = new BounceCS[NUM_BUTTONS];

In setup()

// attach the buttons to the Bounce class                                                         for ( count = 0; count < NUM_BUTTONS; count++) {
    // attach the buttons to the Bounce class
    inputStatus[count].attach(count);       // setup the bounce instance for the current button       inputStatus[count].interval(50);    // interval (bounce) in ms
    [...]
    }
  }

In loop()

// read all input
    // only the first 4 ports are input
    // not needed to store the values, this will get updated in the class
    for ( count = 0; count < 4; count++) {
      CS.portRead(count);
    }
    // this is getting the *cached* results
    for ( count = 0; count < NUM_BUTTONS; count++)  {
      // Update the Bounce instance. this is needed also for the interrupt to manage the temporary changes.                                                                                               inputStatus[count].update();

Yes, cacheDigitalRead is an option, but i have preferred to keep it simple from a (mine) logic perspective.
Still, code can be rewritten, to add a method instead of changing one, i don't see real problem as of now.

The two main question that I have are:

  1. why do you think that this is breaking the current code?
    1.1) It has to be explicitly enabled via .h define (also for the memory footprint)
    1.2) you can still use the digitalRead(pin) -- which will default to cache=false -- or am i fully wrong?
  2. can you point out some typo? I am not getting it... Just to check where I can do better. So I can apply the same to other portion of code.

For the other stuff, like PR squash, fine to me, i can do it once the logic is defined and agreed

Thanks!

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.

2 participants