Skip to content

Countio #2827

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
Merged
merged 19 commits into from
May 13, 2020
Merged

Countio #2827

merged 19 commits into from
May 13, 2020

Conversation

LearnWeaver
Copy link

This is a work in progress, see issue #2815

Any comments or changes needed, please let me know.

tannewt
tannewt requested changes Apr 29, 2020
View reviewed changes
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thanks for this! A couple comments. You'll also want to ensure that the submodules here are the same as in master. A number of us are on Discord if you need live help: https://adafru.it/discord

@LearnWeaver
Copy link
Author

Thanks very much for providing the review and comments. I've now made a few changes:

  • Changed to class Counter
  • Fixed (I think) the submodules
  • Increment on every edge detection
  • Changed the build so that other platforms are excluded (for now)
    The build is still failing on the pyruler board (DE language) - any advice on how to get it down by 60 bytes?

tannewt
tannewt requested changes May 5, 2020
View reviewed changes
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Nice work! Getting close. Just a couple of other things. Thanks!

tannewt
tannewt requested changes May 6, 2020
View reviewed changes
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Getting close! Just a couple more things to address. Once addressed unmark it as a draft and we'll do a final review. Thanks!

@LearnWeaver
Copy link
Author

Really appreciate the time and feedback. Thanks

@LearnWeaver LearnWeaver marked this pull request as ready for review May 7, 2020 02:44
tannewt
tannewt requested changes May 7, 2020
View reviewed changes
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

You misunderstood one of my suggestions so I've been more explicit. Thanks!

LearnWeaver and others added 2 commits May 8, 2020 07:55
@LearnWeaver @tannewt
Update py/circuitpy_mpconfig.mk
94ca233 
Co-authored-by: Scott Shawcroft <scott@tannewt.org>
removed duplicate build param
e947e4e
tannewt
tannewt previously approved these changes May 11, 2020
View reviewed changes
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Ok, this looks good! All it needs is another merge.

@LearnWeaver
Copy link
Author

Thanks again for your help @tannewt - I've merged in master, looks like it breaks on the stm32f405 - with a protomatter issue. Did I merge in the wrong submodule?
Also the catwan_usbhub fails on the locale DE .. (it builds on my machine using English) - any suggestions on what I can do here?

@tannewt
Copy link
Member

tannewt commented May 12, 2020

Try moving into lib/protomatter and checking out 9f71088d2c32206c6f0495704ae0c040426d5764 which is the commit currently in master. That should hopefully fix it.

For the catwan you can just disable COUNTIO for it. It must be tight on space. You can add TRANSLATION=de_DE to the make command if you want to try building it locally.

Thanks!

tannewt
tannewt previously approved these changes May 12, 2020
View reviewed changes
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Everything looks good! I merged in translations from master and will merge this once it passes CI.

tannewt
tannewt approved these changes May 13, 2020
View reviewed changes
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you for this!

@tannewt tannewt merged commit 82fdced into adafruit:master May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhalbert dhalbert dhalbert left review comments

@tannewt tannewt tannewt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LearnWeaver @tannewt @dhalbert