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

Update of Button and Pin input blocks #89

Closed
wants to merge 23 commits into from

Conversation

Amerlander
Copy link
Collaborator

  • adds the event types usable for Button and Pin events
  • merges the onPinPressed & the onPinReleased blocks into a new onPinEvent block.
  • creates a new onButtonEvent block, which replaces the old onButtonPressed one (which actually was click-listener instead of pressed)
  • change the game.ts to use the new onButtonEvent function
  • the old blocks are left on the code side for compatibility

Juri added 13 commits February 13, 2020 22:58
* handle continuous state

* adding shims

* update rendering for continuous servos

* fixing sim

* fix sig

* typo

* fix sim

* bump pxt

* bump pxt
- add Button and Pin event types
- merge onPinPressed & onPinReleased in new onPinEvent function
- create new onButtonEvent function
@Amerlander Amerlander requested a review from pelikhan February 20, 2020 02:33
@Amerlander
Copy link
Collaborator Author

ok, I see. I have to change the tests first.

@pelikhan
Copy link
Member

I recommend having an enum that hides DAL

@Amerlander
Copy link
Collaborator Author

thank you, used the enum now instead of DAL.

I also brought back the deprecated blocks, so they are displayed in block view when old hex files or js code is imported.

@pelikhan
Copy link
Member

I would recommend to create a new extension for these blocks to simplify your life w.r.t. to merging Microbit changes. Also we are trying to keep all the editors compatible so this should probably go into common packages.

@Amerlander
Copy link
Collaborator Author

I'm not sure how this simplifies my life, since I'm still working myself into this and have to figure out how things work in pxt.

  • If I create an extension I would still want to disable the existing button- and pin-pressed-blocks, since I don't want to have four same same but different blocks. So all the docs and tutorials would still need to change in compare to Microbit.

  • common packages could be a way. The buttons.cpp also already has the button events as enum, but I don't know yet what to change there and after how to bind it into pxt-calliope. And still, if Microbit doesn't sneak in to this, it wouldn't be easier to merge their changes later at all?

My reason for these changes are, that the block on Button A pressed in fact listens to click and not down event. And instead oft just renaming or changing the event I thought iit would be nice to have a choice. And since I wasn't able to put the event as an optional parameter into the existing blocks I created new ones to replace the existing.

The Microbit pin and button event listeners have the same mix-up of pressed and click, so I think it would make sense to change it there too.
If it makes sense to put these changes in common packages, could you assist me with getting them in there? And when Microbit makes use of them we can just merge their changes into pxt-calliope which makes sure we're not going apart.

@MKleinSB
Copy link

I remembered this issue: microsoft/pxt-microbit#963
Doesn´t it affect your problem, too?

@pelikhan
Copy link
Member

ready?

@@ -22,13 +22,13 @@ Here's a program that simulates cell life in the LED matrix. Use button ``A`` fo
let lifeChart: Image = null

//Use button A for the next iteration of game of life
input.onButtonPressed(Button.A, () => {
input.input.onButtonEvent(Button.A, DAL.MICROBIT_BUTTON_EVT_CLICK, () => {
Copy link
Member

Choose a reason for hiding this comment

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

Really? Kids should not have to deal with the DAL object. This is super ugly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, this is outdated and changed within 07fe264

@@ -22,13 +22,13 @@ Here's a program that simulates cell life in the LED matrix. Use button ``A`` fo
let lifeChart: Image = null

//Use button A for the next iteration of game of life
input.onButtonPressed(Button.A, () => {
input.input.onButtonEvent(Button.A, ButtonEvent.Click, () => {
Copy link
Member

Choose a reason for hiding this comment

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

double input?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no, the other one is the outdated file, so this is what the kids will deal with.

Copy link
Member

Choose a reason for hiding this comment

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

They have to type Input twice?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah, sorry. Yes, that's wrong. I`ll change that.

@pelikhan
Copy link
Member

This change will make future merging from upstream microbit harder.

@MKleinSB
Copy link

@Amerlander I agree to pelikhan. Wait with the update of the pinevent. The standard Calliope user doesn´t see the difference. It it necessary to keep merging of microbit source easy. The Pinevents will be (perhaps) updated. Look, there is a new issue
microsoft/pxt-microbit#2667

@Amerlander
Copy link
Collaborator Author

@MKleinSB, of course it would make sense to wait when micro bit is changing there something at all. I just wonder whether there is any plan to do this and how I could help with that.

The Issue on the micro bit side was labeled with "next-release", but then removed. They said in 2018 they will introduce a fix next year, but since then there is not really any progress in the issue and also not an answer to my question. I would be glad if the issue can be closed on the side of micro bit, and we can just check in.

The issue microsoft/pxt-microbit#2667 doesn't seem to target this, since its more about when a click event is triggered and when it is a long click and when its not a click at all (a down event where another event comes in between before the up event is not triggered as click anymore). And there the behavior seems to differ from simulator to hardware.

@MKleinSB
Copy link

@Amerlander If you stay working on makecode.calliope it´s ok ( for me ;-) )

@pelikhan
Copy link
Member

I would recommend to create a new extension that produces the better blocks then merging would be easier. We should setup a call to discuss these things.

@Amerlander
Copy link
Collaborator Author

@MKleinSB I'm pretty sure I'll stay, even when this isn't the only thing I'm working on.

@pelikhan having a call to discuss this sounds great.

This was referenced May 5, 2020
@Amerlander Amerlander closed this Aug 19, 2020
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.

3 participants