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

Control digitalPin status using bits of a uint32_t #61

Merged
merged 4 commits into from
Mar 9, 2022

Conversation

CaioPellicani
Copy link
Contributor

No description provided.

Copy link
Owner

@bxparks bxparks left a comment

Choose a reason for hiding this comment

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

Hi, I left you some minor logic error fixes and improvements.

But the more important question is: Why do you want this functionality? Because in a real microcontroller, a digitalWrite() to a specific pin does not affect the value of the digitalRead() on that pin. They are completely decoupled from each other. I'm not sure that EpoxyDuino should add a semantic coupling between those 2 functions that doesn't exist in real life.

void digitalWrite(uint8_t /*pin*/, uint8_t /*val*/) {}
void digitalWrite(uint8_t pin, uint8_t val) {
if( val == HIGH ){
arduinoPins |= ( 1LU << ( pin - 1 ) );
Copy link
Owner

Choose a reason for hiding this comment

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

pin == 0 is a valid pin, so all the pin - 1 should be just pin

arduinoPins |= ( 1LU << ( pin - 1 ) );
}
if( ( val == LOW ) && ( digitalRead( pin ) ) ){
arduinoPins ^= ( 1LU << ( pin - 1 ) );
Copy link
Owner

Choose a reason for hiding this comment

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

this should be

arduinoPins &= ~(1LU << pin);

}

int digitalRead(uint8_t pin) {
return ( arduinoPins & ( 1LU << ( pin - 1 ) ) ) == ( 1LU << ( pin - 1 ) );
Copy link
Owner

@bxparks bxparks Mar 2, 2022

Choose a reason for hiding this comment

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

Since we are testing for only a single bit, we can simplify this to:

return (arduinoPins & (1LU << pin) != 0;

This generates fewer machine instructions because it doesn't have to test against an extra argument.

@CaioPellicani
Copy link
Contributor Author

Hello,
I use this changes to track the state of each pins, so I can test it or print on terminal.
This was helpful to me.
Thanks for the logic correction and tips

@bxparks
Copy link
Owner

bxparks commented Mar 3, 2022

I can see the convenience of being able to set the value returned by digitalRead() for testing. But I don't think we should use digitalWrite() for that purpose.

How about we create a new method, call it something like digitalReadValue(uint8_t pin, uint8_t value)? The client code will need to use an #ifdef conditional, otherwise the code will not work in a real Arduino environment:

#if defined(EPOXY_DUINO)
   digitalReadValue(xxx, yyy);
#endif

But if the code is meant to run only on EpoxyDuino, then the conditional guard can be omitted.

If this is agreeable to you, then I would like to make some commits to your branch to polish the PR for inclusion: fix bugs, add error checks, apply consistent coding style, add documentation, etc.

@CaioPellicani
Copy link
Contributor Author

Sure, I think this is a great idea!

This is my first pull request, so maybe I will need some help in the process

@bxparks
Copy link
Owner

bxparks commented Mar 3, 2022

Normally the PR author (you) would make changes requested by the reviewer (me), and then do a git push of the new changes so that the reviewer can see the changes. But it's far easier and faster to do the changes myself than to describe those changes in lots of words. GitHub has a feature which allows the reviewer to make changes directly on the PR author's branch. That also allows me to preserve the authorship of the PR. That's the theory, I think I used this feature once or twice before, but I can't remember. I'll give it a shot tomorrow.

@bxparks
Copy link
Owner

bxparks commented Mar 9, 2022

I pushed 3 commits into your branch. Please take a quick look. If this is acceptable for you, I can merge this in this PR.

@CaioPellicani
Copy link
Contributor Author

that's great, is more than acceptable

@bxparks bxparks merged commit c217a98 into bxparks:develop Mar 9, 2022
@bxparks bxparks mentioned this pull request Mar 28, 2022
kwisii pushed a commit to kwisii/EpoxyDuino that referenced this pull request Aug 4, 2022
kwisii pushed a commit to kwisii/EpoxyDuino that referenced this pull request Aug 4, 2022
kwisii pushed a commit to kwisii/EpoxyDuino that referenced this pull request Aug 4, 2022
kwisii pushed a commit to kwisii/EpoxyDuino that referenced this pull request Aug 4, 2022
kwisii pushed a commit to kwisii/EpoxyDuino that referenced this pull request Aug 4, 2022
kwisii pushed a commit to kwisii/EpoxyDuino that referenced this pull request Aug 4, 2022
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