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

on touched, on released, unexpected behaviour #963

Open
DavidWhaleMEF opened this issue Jul 24, 2018 · 24 comments
Open

on touched, on released, unexpected behaviour #963

DavidWhaleMEF opened this issue Jul 24, 2018 · 24 comments
Labels

Comments

@DavidWhaleMEF
Copy link

Describe the bug
Using on touched and on released doesn't seem to operate with simple behaviour, as I would expect.

To Reproduce
Use this code for a dead-man's-handle (train driver) in the makecode simulator

screen shot 2018-07-24 at 17 24 14

Expected behaviour
I expect when I touch P0 to see a smiley to let me know the train is safe to drive, and when I release my touch on P0 it scrolls 'STOP THE TRAIN'

Desktop (please complete the following information):
Mac OSX 10.10.5 Google Chrome

Additional context
I am porting some simple scripts from Touch Develop in to makecode for one of our content partners. This was a lovely and simple example in Touch Develop, but it seems really hard to get it to work reliably in MakeCode.

@DavidWhaleMEF
Copy link
Author

Actual Behaviour
When I run it out of the box first time, I touch and see smiley, I release and see STOP THE TRAIN then I get a smiley again. But my finger is still released and I have not touched it again

@DavidWhaleMEF
Copy link
Author

This might be a related issue: #746

@DavidWhaleMEF
Copy link
Author

This is how I worked around it

screen shot 2018-07-24 at 17 24 24

@DavidWhaleMEF
Copy link
Author

@jaustin has worked through this at the lower event-based level to quite some detail and has a view on what the issue is here - Jonny, can you paste your experiment results here?

@DavidWhaleMEF
Copy link
Author

Also @microbit-mark has tried some test cases and sees the same behaviour. Mark, if you have anything to add, can you paste here? Thanks!

@DavidWhaleMEF
Copy link
Author

Also, this
screen shot 2018-07-25 at 12 19 10
simple script doesn't work either?

@DavidWhaleMEF
Copy link
Author

I also get strange and unexpected behaviour from this script...
screen shot 2018-07-25 at 12 25 01

@DavidWhaleMEF
Copy link
Author

DavidWhaleMEF commented Jul 25, 2018

This doesn't yield any expected behaviour either, it does the same (wrong) thing for all 4 settings of the event type.

screen shot 2018-07-25 at 12 27 34

@DavidWhaleMEF
Copy link
Author

And this doesn't work for any of the 4 event types either

screen shot 2018-07-25 at 12 29 51

@DavidWhaleMEF
Copy link
Author

@microbit-mark @jaustin Do we have any documentation saying what our expected (simple) behaviour is for pressed and released?

@DavidWhaleMEF
Copy link
Author

For context, I am rewriting a whole stack of one of our content partner resources, porting code from Touch Develop to MakeCode. It's being done on a very tight deadline and is proving impossible in this area, Below is the (simple and obvious) TouchDevelop code I am struggling to rewrite in MakeCode...

screen shot 2018-07-25 at 12 47 28

@microbit-mark
Copy link
Contributor

Hey @abchatra can you prioritise this? As @DavidWhaleMEF notes it used to work in Touch Develop and we can't update our resources for Makecode without being confident this works. Do you have any examples of pin touch & release that work both in simulator and on micro:bit?

@DavidWhaleMEF
Copy link
Author

Some of these scenarios are hampered by the fact that the simulator UX for touch was not as I expected (the analog pin feedback was confusing me).

I will re-test each of the above scripts and report back, when I get a chance. I think there is a bug with the event based handling (Jonny has some notes on this), but the if condition ones do seem to work now, I think.

@abchatra
Copy link
Collaborator

If Pin pressed and released doesn't work on the hardware then it is likely DAL issue and not in MakeCode

@DavidWhaleMEF
Copy link
Author

So @abchatra it turns out there are two issues here

  1. UX that confused the user (me)

When using the touch/pressed blocks, the simulator still runs concurrently the analog pin animations. This confused me as I thought I had to get the analog value high enough for touch to be registered. Check with @jaustin but we might want later to request that the analog animation is disabled when in touch mode, to prevent this user confusion.

  1. the event handlers have some semantics that make them hard to understand and use.

If using the pin touched/pin released event handlers, we get correct semantics as per the DAL for pressed, and consistent behaviour as per button pressed - i.e. the press is actually triggered on release (due to debouncing semantics). The release semantics are hard to understand though, try the below script and see if you can get it to do anything sensible!

screen shot 2018-07-26 at 10 45 36

Suggested actions
1a) I think I'll work with @microbit-mark to write a support kbase describing the behaviour better (perhaps you can suck some of this up into the docs too)

1b) Can you chat with @jaustin and @microbit-mark at the next support sync, and see if we need to consider disabling the analog pin animation in the simulator when touch mode is in use, to prevent user confusion?

  1. Can you guys try this sample script out and provide a better description of released? Also @jaustin did some event tracing of that use case and found some strange event behaviour that we couldn't explain, perhaps @finneyj could explain that with his DAL perspective?

@martinwork
Copy link
Contributor

With the on-pin-pressed/released script....

When you put your finger on, the DAL fires MICROBIT_BUTTON_EVT_DOWN, which doesn't trigger either of these "on..." handlers.

When you take your finger off, the DAL fires 2 events:

  1. MICROBIT_BUTTON_EVT_UP, which triggers on-pin-released.
  2. Either MICROBIT_BUTTON_EVT_CLICK or MICROBIT_BUTTON_EVT_LONG_CLICK, depending on how long your finger had been touching. Only MICROBIT_BUTTON_EVT_CLICK triggers on-pin-pressed.

So the display changes only when you take your finger off and the result is clear or heart depending on how long it had been on.

@microbit-mark
Copy link
Contributor

That would tally with the experience on the hardware.

@DavidWhaleMEF
Copy link
Author

So, the wider question then is - does this make sense to an 11 year old?

@martinwork
Copy link
Contributor

It doesn't make sense to me! Why use "pressed" for an event triggered when the pin gets released? on-pin-pressed and on-pin-released are very similar events. Is there an "on" handler for MICROBIT_BUTTON_EVT_DOWN?

This makes no sense unless you know that on-pin-pressed is badly named:
capture

Related:
microsoft/pxt#3509
microsoft/pxt#2495
microsoft/pxt#2445
microsoft/pxt#540

@DavidWhaleMEF
Copy link
Author

@abchatra @jaustin care to comment here?

@abchatra
Copy link
Collaborator

I completely agree, they are badly named. Pressed is actually Click and I believe is the only useful block for students. We didn't want to change any API for last academic year, but we can do a breaking change next year.

@DavidWhaleMEF
Copy link
Author

Thanks @abchatra see my notes in #972 about a suggested process by which we could allow fixes and innovation that breaks API's in a way that has minimal impact on our users.

@Amerlander
Copy link
Contributor

I did the necessary changes in pxt-calliope: microsoft/pxt-calliope#89

I created new blocks, where the input-event can be selected.

The changes are non-breaking, since the functions are still there and the old blocks can be rendered. But they are not shown nor selectable in the IU.

If wanted I can prepare a PR for pxt-microbit too. If this should go into common-packages I'll need some advice on how this should be implemented there.

I was also thinking about going further and combine the real-touch functionality (https://github.com/microsoft/pxt-microbit-buttons) and also was thinking about combining the pin-input and button-input blocks, but for now left both out.

@kiki-lee
Copy link
Contributor

Adding a +1 here. Super frustrating that the simulator works correctly and the micro:bit does not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants