Skip to content

New Guide - rock, paper, scissors game for CLUE and CPB. #1185

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 24 commits into from
Aug 18, 2020
Merged

New Guide - rock, paper, scissors game for CLUE and CPB. #1185

merged 24 commits into from
Aug 18, 2020

Conversation

kevinjwalters
Copy link
Contributor

@kevinjwalters kevinjwalters commented Jul 22, 2020 •
edited
Loading

Some CircuitPython files for a new guide. Four directories, three different versions of the game plus unit tests:

  • very-simple
  • simple
  • advanced - this includes an rps subdirectory holding wav files and a bmp file
  • tests - two units tests which can run under CPython

@kevinjwalters
Copy link
Contributor Author

kevinjwalters commented Jul 23, 2020 •
edited
Loading

All ok now. (Please don't merge yet - I've got a little bit more minor tidying to do, will do this today.)

@kevinjwalters kevinjwalters changed the title New Guide - rock, paper, scissors game for CLUE and CPB. Don't merge yet - New Guide - rock, paper, scissors game for CLUE and CPB. Jul 23, 2020
@kevinjwalters kevinjwalters changed the title Don't merge yet - New Guide - rock, paper, scissors game for CLUE and CPB. New Guide - rock, paper, scissors game for CLUE and CPB. Jul 23, 2020
@kevinjwalters
Copy link
Contributor Author

Good to go, now.

@kevinjwalters
Copy link
Contributor Author

@TheKitty might be interested in this.

@TheKitty TheKitty requested a review from dhalbert August 4, 2020 18:47
@TheKitty
Copy link
Collaborator

TheKitty commented Aug 4, 2020

@dhalbert - would you please double check this use of Bluetooth between devices per this guide in moderation https://learn.adafruit.com/admin/guides/2998/editor

@kevinjwalters
Copy link
Contributor Author

FYI, there's some notes on the innards in the draft Learn Guide with current title of "CLUE Rock, Paper, Scissors Game using Bluetooth"

@kevinjwalters
Copy link
Contributor Author

I've just improved the error reporting in case the user has missing audio files. Will have a commit for that on the way within 20 mins.

Kevin J Walters added 2 commits August 4, 2020 21:36
Correcting/improving the error reporting for mulitple missing audio f…
304aa80 
…iles from SampleJukebox constructor with new SampleJukeboxError exception.
pylint vs whitespace
3806b2f
dhalbert
dhalbert reviewed Aug 4, 2020
View reviewed changes
Copy link
Contributor

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

This is an amazing amount of work! I am sorry you have to deal with the storage foibles.

Could you have used a more ad-hoc advertisement, instead of all the Manufacturer data field stuff? Would that have simplified things?

TheKitty and others added 6 commits August 5, 2020 09:09
@TheKitty
Merge branch 'master' into rockpaperscissors
a295bc4
Renaming files to a more specific audio_files.
ea82f2b 
Removing error_output in SampleJukebox constructor as it is no longer used (exception now lists all missing files).
Changing evaluateRound() to return value to a form of enumerated valu…
758280b 
…e based on const() numbers for WIN, DRAW, LOSE, INVALID.

Maintains the use of booleans on the call to rps_display.showPlayerVPlayer(). adafruit#1185
Converting filter() to list comprehension with trailing if filter syn…
e3dd2d2 
…tax based on review. adafruit#1185
Removing the C-esque pad removal in strUnpad() and replacing with rst…
95acc97 
…rip based on review.

Note: CP 5.3.0 bytes(xx) always make a new object. adafruit#1185
Switching to the new match_prefixes for Advertisement matching which …
2612984 
…replaces prefix.

Changing to matching using the library rather than the match_locally enabled code and a few fixes around that dusty code path.
match_prefixes came up in the review adafruit#1185
@kevinjwalters
Copy link
Contributor Author

kevinjwalters commented Aug 6, 2020 •
edited
Loading

  • I picked up 20200806 libs and I now have one text Label that has changed location and is half off the screen. I need to sort that out.

Kevin J Walters added 2 commits August 7, 2020 18:11
Rearranging the text setting for the summary Label on round results s…
ff6fffe 
…creen to set in constructor rather than later via property. This is neater but main motivation is to work around a Label y position bug.
Moving points values out to constants.
81fbdd7 
Squashing some assignments onto one line.
@kevinjwalters
Copy link
Contributor Author

kevinjwalters commented Aug 8, 2020 •
edited
Loading

On the format of the application's Advertisement various packets I'm not sure. I carefully selected ID values to achieve a certain order within the ManufacturerData based on currrent CircuitPython hash table algo. That ordering allows the prefix matching to work when peering into the ManufacturerData. That's clearly fragile, bad practice, sets a bad example and almost guaranteed to break if/when CircuitPython is changed to match the CPython dict() ordering. I think I could use the multi-field feature in ManufacturerDataField but that wouldn't be elegant for optional fields.

I intended to watch this space and update the code with a better solution when one appeared or I found one.

If ordering within ManufacturerData could be controlled then my use of it would be ok. There's a few ways to offer that if that's generally desirable:

  1. Order as field values are set.
  2. Order as fields are declared in the sub-class of Advertisement
  3. Some explicit specification of full / partial ordering requirement

I like the sound of 2 myself but 1 happens to be very easy to implement as I think it could be achieve with just replaced {} with an OrderedDict() at https://github.com/adafruit/Adafruit_CircuitPython_BLE/blob/9efa23a823df695421dcc766832f397a236ac1f0/adafruit_ble/advertising/standard.py#L223 ?

@TheKitty
Copy link
Collaborator

@kevinjwalters Is it at the merge point now?

@kevinjwalters
Copy link
Contributor Author

@TheKitty Almost, would be good to get a view on #1185 (comment) from @dhalbert

@dhalbert
Copy link
Contributor

If ordering within ManufacturerData could be controlled then my use of it would be ok. There's a few ways to offer that if that's generally desirable:

  1. Order as field values are set.
  2. Order as fields are declared in the sub-class of Advertisement
  3. Some explicit specification of full / partial ordering requirement

I like the sound of 2 myself but 1 happens to be very easy to implement as I think it could be achieve with just replaced {} with an OrderedDict() at https://github.com/adafruit/Adafruit_CircuitPython_BLE/blob/9efa23a823df695421dcc766832f397a236ac1f0/adafruit_ble/advertising/standard.py#L223 ?

I think 1 is fine with an OrderedDict. But I would say three things:

  1. You don't have to filter based on the Manufacturer Data fields. You can just filter on the manufacturer (or not at all), and then filter "by hand" later.
  2. You don't have to adhere to that format of advertisement.
  3. You can merge the Mfr Data fields into a single structure field.

Changing the advertisements used by Advanced RPS Game to have manufac…
ca15fb3 
…turer data declared and set in the desired serialization order.

Changing the id numbers on manufacturer data in the advertisements to use a broadly sequential set.
Removing unused RPS_VERSION. adafruit#1185
@kevinjwalters
Copy link
Contributor Author

kevinjwalters commented Aug 12, 2020 •
edited
Loading

I've updated my code and thrown in some TDD using new tests/test_rps_advertisements.py. I put in adafruit/Adafruit_CircuitPython_BLE#97 for the OrderedDict enhancement which passes on that test.

@kevinjwalters
Copy link
Contributor Author

@TheKitty This is good to merge now.

dhalbert
dhalbert approved these changes Aug 18, 2020
View reviewed changes
Copy link
Contributor

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Thanks! This is all set now, because the OrderedDict fix has gone into adafruit_ble, and been released.

@dhalbert dhalbert merged commit e85939b into adafruit:master Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhalbert dhalbert dhalbert 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
@kevinjwalters @TheKitty @dhalbert