Make pin parameters variable

[sae220]

#5558
microsoft/pxt#10015

[abchatra]

@carlosperate @jaustin can you review this and let us know if these changes to blocks are ok?

[carlosperate]

Is there any user facing impact to the blocks and/or the javascript API? If so, can we get a before and after screenshot?

[sae220]

@carlosperate

[nevergone]

Do you know any good methods for testing equality that children can use?
microsoft/pxt#10015 (comment)

[microbit-carlos]

Thanks for the screenshot @sae220! What about the Javascript pin API, how does the code change?

[sae220]

@nevergone

Do you know any good methods for testing equality that children can use?

Sorry, but I can't understand well. You mean you want to check for equality of different pins?

@microbit-carlos

What about the Javascript pin API, how does the code change?

As far as I know, there are no code changes in JavaScript.

[nevergone]

@sae220

I have two variables with pin.
First I want to check that they contain the same pin. And then that the variable contains Pin2. Just as the contents of other variables can also be compared.
You have done a lot of work, for which I thank you very much.
If this can be solved, I think it will be perfect.

[sae220]

@nevergone

So you mean you want to compare obviously inequal pins (like DigitalPin.P0 and DigitalPin.P1)?
As I shew before, we can compare pins when we use array (like DigitalPin and DigitalPin.P1).
Isn't it enough for your purpose?

Sorry for my poor English.

[nevergone]

@sae220

Some pins are stored in a variable. How do we know afterwards which pin is in the variable? How can we check if a specific pin is present in the variable?

[sae220]

@nevergone

I'm very sorry, but I can't understand the situation well.
Could you give me some example of code (blocks or JavaScript)?

[nevergone]

@sae220
I tried it and now it works perfectly. Thanks for all your work! :)

input.onButtonPressed(Button.A, function () {
    if (var1 < var2) {
        basic.showIcon(IconNames.Heart)
    }
    if (var1 < DigitalPin.P10) {
        basic.showIcon(IconNames.SmallHeart)
    }
})
input.onButtonPressed(Button.AB, function () {
    if (var1 == var2) {
        basic.showIcon(IconNames.Duck)
    }
    if (var1 == DigitalPin.P10) {
        basic.showIcon(IconNames.Tortoise)
    }
})
input.onButtonPressed(Button.B, function () {
    if (var1 > var2) {
        basic.showIcon(IconNames.Yes)
    }
    if (var1 > DigitalPin.P10) {
        basic.showIcon(IconNames.Happy)
    }
})
input.onGesture(Gesture.Shake, function () {
    pins.digitalWritePin(var1, 1)
})
let var2 = 0
let var1 = 0
var1 = DigitalPin.P0
var2 = DigitalPin.P1

[sae220]

@abchatra
I fixed some test code. Could you review this again?

[riknoll]

LGTM!

This needs upgrade rules to ensure backwards compat with old projects
Also needs new docs pages for the two new blocks

...but we can just file an issue for that and do it later

[abchatra]

@sae220 thanks for contribution. There are few follow ups: upgrade rules for existing projects and doc pages update. If you can file issues for the same it would be great. If you want to tackle the upgrade rules feel free to do so as well.

[sae220]

@abchatra @riknoll
OK. I'll upgrade soon, but I don't know how I can upgrade rules for existing projects to ensure backwards compat.
Could you let me know the helpful commit?

[riknoll]

@sae220 it's all in this file: https://github.com/microsoft/pxt-microbit/blob/master/editor/patch.ts

basically just need to add some code that converts the old xml to the new one, just like you did for the test code

[abchatra]

Also @sae220 you should be able to see the new block in /beta now. Greatly appreciate the work.

[jaustin]

@abchatra would you like us to raise a new issue or continue talking here for feedback on these changes?

Two things that seem confusing right away:

1. The distinction between the getter read digital pin and just digital pin - given we don't use the read language on getters elsewhere in micro:bit land:
   One of these is not like the other (but it's not the last one, which looks like it's different):

2. I'm not sure I understand the impact of the fact a user can now swap an analog pin into a digital operation block - it seems confusing:

Should there be some typing that means this isn't possible? The blocks now read in a more cumbersome and confusing way

[sae220]

@jaustin
You've got a point. This pull request may be thoughtless.

But it isn't caused by this pr that the getter method digital read pin includes read. I don't have any idea to solve this.

To solve other problem, what do you think about the integration of digital pin and analog pin?
Using blocks error list (but experimental), we can restrict usable pins.

[sae220]

@abchatra
What do you think about this?

[microbit-carlos]

@abchatra looks like the change in the blocks is causes issue loading older projects, like in #5728.

Even if the generated code is identical the change in the blocks themselves seems to be causing this issue.
Has MakeCode already implemented a way to "convert old code" for previous block updates? When other blocks, like the music blocks, have been refactored, how did we managed importing older programmes into newer versions of MakeCode?

[abchatra]

We can fix the update issues.

[sae220]

@microbit-carlos @abchatra
I created pull request to convert old code.
#5712

[microbit-carlos]

Thanks for all the work on these PRs @sae220!

I have a few suggestions, which might be worth considering as I think it'd likely affect #5712 as well.

To reduce the block visual difference, and word redundancy (pin [P0] vs pin [digital pin P0]):

MakeCode v6	This PR

We can probably create a separate block with just the pin name, that is only used as a shadow block for other pin blocks.
These blocks wouldn't be available on their own, and they could not be "dragged out" from the other pin blocks (the currently implementation doesn't let you drag-out the shadow block, so that's already done).

So my suggestion to this PR would be something like this:

This PR	Suggestion

And so the change between MakeCode v6 and v7 would be very small:

MakeCode v6	Suggestion

@abchatra having this smaller digital/analog pin block to serve only as a shadow block something you'd consider accepting?

---

We can still keep the new pin blocks, as these are new and they also need to be self explanatory on their own:

---

Another change that would be needed is to revert back the pulse event block, or to change it in a way that doesn't let the user add a variable for of the pin:

This is because unfortunately the code generation can end up placing the event handler code on top of the "on start" code, and so we might end up with an undefined variable:

---

As a separate conversation we might need to consider if the "analog pin" block can be improved in the future as well, as something like this code snippet should not work, but MakeCode doesn't show any errors and it even works in the simulator:

And to clarify, this was not introduced in this PR, it has been like this in MakeCode for a very long time, but this PR makes it a bit more obvious as a user creating a list of pins now needs to choose between selecting a "digital pin" vs an "analog pin".

I've created this issue to discuss this problem:

• Digital pin vs analog pin blocks #5733

[sae220]

@microbit-carlos
About the first suggestion, I'll create new pull request to shorten new blocks after discussion in #5733.

To solve second point, how about fix compiling code to bring on-start code top?