Skip to content

Provide digitalPinToPort and related functions #197

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

Merged
merged 3 commits into from
Nov 10, 2020

Conversation

jgfoster
Copy link
Member

@jgfoster jgfoster commented Nov 6, 2020

I don't understand this very well, but it appears that some boards use memory-mapped I/O to read/write pins. To support that style of code this PR adds an array that can hold pin values. A more elaborate approach would be to connect to the pin logs, but this seems like a decent first step (and allows some files to compile that didn't compile before). Fixes #193.

It appears that some boards use memory-mapped I/O to read/write pins. To support that we have an array that can hold pin values. A more elaborate approach would be to connect to the pin logs, but this seems like a decent first step (and allows some files to compile that didn't compile before).
@ianfixes
Copy link
Collaborator

ianfixes commented Nov 7, 2020

I'm inclined to merge this, as the unit tests look sensible. The GODMODE mmapPorts is intriguing as well, I'm curious whether that will get me unstuck on #120 (comment)

I'll merge this by Monday unless @per1234 or @matthijskooijman see any dealbreakers in there.

@matthijskooijman
Copy link
Collaborator

I'm not sure if this is the best approach, but it's nothing that can't be improved on later, I think.

For completeness, I think there's also a similar macro to get at the port's direction register, that could maybe be added (or later, of course).

It's a bit memory-inefficient (allocating 1 byte per pin to store one bit), but that's not really an issue when running CI either.

One thing that I'm unsure about is the 256 extra pins being allocated as "padding". I'd say that pin numbers run from 0 to MOCK_PINS_COUNT or so, so why the extra 256? There's a comment, but I don't understand it (and haven't looked at the __MK20DX128__ that it references, not sure where to look).

@jgfoster
Copy link
Member Author

jgfoster commented Nov 7, 2020

@jgfoster
Copy link
Member Author

jgfoster commented Nov 8, 2020

I've removed the padding and limited the code to AVR only. Any other suggestions?

@jgfoster jgfoster changed the title Possible fix for #193 Provide digitalPinToPort and related functions Nov 8, 2020
@matthijskooijman
Copy link
Collaborator

Responding to comments in #193, which are indeed better placed here.

That can certainly be done. As to the "else" portion, should I just leave them undefined since I'm not in a position to fully implement them for every architecture?

Yes, that's what I'd do for now. Things can be added for other architectures, but that requires careful review of what the relevent Arduino cores implement. Also, I'd expect that AVR is the most likely one to have sketches that use this semi-internal API, so this would cover most cases.

As for the padding and other boards, I'd think that supporting those particular code and Ethernet library, would be a separate thing (if it's really desirable at all).

From another quick glance, this looks ok to merge to me.

@ianfixes ianfixes merged commit 107ec9d into Arduino-CI:master Nov 10, 2020
@ianfixes
Copy link
Collaborator

Thanks for the discussion on this!

@ianfixes ianfixes added this to the OpenAcidification milestone Nov 10, 2020
@jgfoster jgfoster deleted the digitalPinToPort branch November 11, 2020 01:03
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.

Undeclared identifiers digitalPinToPort and digitalPinToBitMask
3 participants