Safer behaviour when the mode of a pin changes. Fix up PWM implementation

[markshannon]

Rework the pwm module to allow smooth transitions when period or duty cycles are changed.
Make music module use our pwm (not the DAL's pwm).
Modify the pin and related modules to ensure that attempting to use a pin when it is already used for another purpose, raises an exception.

For example:

>>> pin5.write_analog(300)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Pin 5 in button mode
>>> pin6.write_analog(300)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Pin 6 in display mode
>>> display.off()         
>>> pin6.write_analog(300)
>>> display.on()          
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Pin 6 in analog mode
>>> pin6.write_analog(0)  
>>> display.on()        
>>> 

Fixes #335, #337, #331, #288.

[dpgeorge]

There are a lot of changes here.... what is the delta of code size and RAM usage? I see you needed to reduce the heap by about 150 bytes, and that's going to potentially break some large scripts.

[whaleygeek]

Hmm,

Changes in RAM usage are going to be a nightmare, especially if user
scripts that once worked now no longer fit or they crash when they run,
when a user upgrades to a more recent version of MicroPython. It will be a
terrible experience for users.

I know that the RAM utilisation story is complex, and especially
challenging with MicroPython due to the need to store the Python bytecodes
in RAM. But I think it needs good management now that the whole system is
out of beta (you did know that we're out of beta I presume? The beta tag
was removed from the website w/c 12th Sept). So this is now released
product!

Clearly the heap is one area we could set a high water mark on and signpost
in the code so that it never changes in the future. Other static RAM
utilisation could be managed by perhaps setting an overall static RAM high
water mark that indicates exactly how much RAM is left for user programs
(and us policing this guaranteed amount of static RAM in all future builds).

Actual runtime RAM utilisation of each feature is much harder to control. I
wonder if the only way we could control this is to have a representative
set of test scripts that we must run before committing features that affect
runtime RAM utilisation?

Damien you're doing a great job to spot this, but it feels to me like there
needs to be more signposts and checks in place so that others can police it
too, otherwise we are always highly reliant on you checking every single PR
carefully.

On 20 September 2016 at 05:11, Damien George notifications@github.com
wrote:

There are a lot of changes here.... what is the delta of code size and RAM
usage? I see you needed to reduce the heap by about 150 bytes, and that's
going to potentially break some large scripts.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#345 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AD5cAWOpUP2e9mPxHw38CCdLW4WDwmiTks5qr1zxgaJpZM4KAi2L
.

[markshannon]

@dpgeorge Thanks for spotting the reduced heap size, I'd just chosen the lowest option when rebasing with the intention of fixing it up later, and then forgotten.
The available heap has in fact increased by a tiny bit.

Filesystem size will have reduced, but that's only temporary as this takes us very close to being able to drop the DAL.

[dpgeorge]

Filesystem size will have reduced, but that's only temporary as this takes us very close to being able to drop the DAL.

If we also drop mbed then we can reclaim a whole 2k of RAM, which would be the idea outcome.

[markshannon]

I plan to add a test-suite for this in the next few days.
You may care to hold off reviewing this properly until then.

[markshannon]

OK, I've added the test and it passes.

[dpgeorge]

@markshannon very nice work! I like that there are more "const" objects (ie less RAM usage) and that there's no longer a need for DAL Pin and Button, which take up lots of RAM. It seems that the code size actually decreases by about 500 bytes (probably due to less functions from DAL needed in the link stage).

One point re the API: I think it should be allowed to do pin.write_digital even if the pin is in analog mode. There's no real need to force users to do pin.write_analog(0) then pin.write_digital(x).

Also, please rebase :)

[dpgeorge]

@markshannon ping, see above comment.

[markshannon]

Found some time to finish this at last.
I've extended the tests to check that repeated switching of modes still leaves things in a sane state.
Uses qstrs rather than ints to identify modes, which should make things more robust when adding more modes (like neopixel or audio-record).

(No need to rebase, github can do that for us now)

[dpgeorge]

Uses qstrs rather than ints to identify modes, which should make things more robust when adding more modes

It's just as robust to add a new member to an enum, so I don't see a problem using ints here.

Why expose the pin.get_mode() method at all? Users should already know the mode of the pin because they are using the pin for some purpose.

Regarding get/set pull: for the constants PULL_DOWN etc why not use the nrf register values directly? Then there is no need for a lookup table to convert between (-1,0,1) and the NRF_GPIO_PIN_xxx value. The values of these constants (exposed on the Python side) should anyway be opaque to the user, so it's ok to use the register values.

And from a consistency point of view, if get_mode is going to be there in the API then it should be similar to get_pull, ie return an integer which corresponds to some constant.

[markshannon]

I should have been a bit clearer, it is not ints that are less robust than qstrs. It is using an external index which cannot be verified. Searching the array of modes, looking for a marker, whether it is a string or an int, is more robust than an unchecked indexing operation.
In other words we cannot tell whether pin_modes[PINMODE_DISPLAY] is in fact the mode for the display without inspecting it. So we are better off just searching the modes.
We need a qstr in the mode for error reporting, so we might as well use it as an index.

Strings are also self describing.

>>> p.get_mode()
"display"

is a lot use informative than

>>> p.get_mode()
7

The reason for using -1, 0, 1 rather than the nordic-defined constants is similar.
The values are opaque when setting them, but not when getting them.
Which is of 0,2,3 is up or down? It is rather more obvious with -1, 0, 1.
We could change set_pull/get_pull to take strings, but enums do have the advantage of offering some static typing thanks to auto-completion.

[dpgeorge]

It is using an external index which cannot be verified. Searching the array of modes, looking for a marker, whether it is a string or an int, is more robust than an unchecked indexing operation

To validate an index you just check that it's within a certain range (eg idx >= 0 && idx <= 10).

My other main point is that you don't need to expose pin.get_mode().

The reason for using -1, 0, 1 rather than the nordic-defined constants is similar.
The values are opaque when setting them, but not when getting them.

They are also opaque when getting. You should test against known values, eg pin.get_pull() == pin.PULL_UP.

[markshannon]

You can't validate an index just by checking it is in range. E.g.

enum {
     DISPLAY_T,
     SPEECH_T
};
pinmode_t modes = {
    { "speech", ... },
    { "display", ... },
};

which is awfully easy to do when adding new values to a larger array.
I want to expose pin.get_mode() for testing. Quite happy to leave it undocumented.
pin.get_pull() is not opaque if you use it in the repl 😉
What is the advantage of using the Nordic values? The lookup table is tiny.

[dpgeorge]

It's possible to protect against incorrect ordering in an array using a C99 feature:

pinmode_t modes[] = {
    [DISPLAY_T] = {...},
    [SPEECH_T] = {...},
};

We use this quite a bit in uPy core.

Well, anyway, if you want to keep the index as a qstr then I'm fine by it, especially since it's used to print a nicer error message.

What is the advantage of using the Nordic values? The lookup table is tiny.

The fact that you need an LUT at all. And there's extra translation code in get_pull() as well that you wouldn't need.

[markshannon]

I'd like to get this merged as it is blocking other work. I really don't think it is worth quibbling over a few bytes in a lookup table, as we will save kilobytes when we remove the superfluous parts of the DAL.

[dpgeorge]

I'd like to get this merged as it is blocking other work. I really don't think it is worth quibbling over a few bytes in a lookup table, as we will save kilobytes when we remove the superfluous parts of the DAL.

The more bytes we save now the better. It's hard to take things away in the future (because users rely on them).

I still think that making artificial pull values (-1, 0, 1) is not a good idea and wastes bytes. If I were a new user and did pin.get_pull() and got 0, I'd think it was pull down (since 0 is a low logic level). If I got -1 I'd be confused and think it was perhaps pulling it to a negative voltage.

My proposal is as before: the constants PULL_UP, PULL_DOWN and NO_PULL should be the corresponding nordic register values, and users should use pin.get_pull() == pin.PULL_UP to do checks.

What do other people think?

[deshipu]

Personally, I would expect NO_PULL to map to None, simply because that's the implied default value when no parameter is passed to the pin's initialization. As for the other two values, constants sound reasonable, as long as they evaluate to True in boolean context.

[ntoll]

Regarding @dpgeorge's question...

I'm wearing my teacher's hat here - the most important thing is that it's obvious what's going on and that "what's going on" is related to current conventions and practices (so such skills and knowledge can be transferred).

Given the above and upon careful reflection I prefer Damien's suggestion for the following reasons:

• PULL_UP, PULL_DOWN and NO_PULL are all unambiguous in what they mean.
• The use of such terminology is a positive thing since it's educating users how to describe things in the expected way.
• That such constants correspond to Nordic register values is also a good thing since anyone who wants to "look behind the curtain" and see what's going on will be able to line up such values with any specifications from Nordic.

I hope this makes sense..? It would certainly make my life as a teacher and learner easier.

Also, if what Damien says is true, it's more efficient in terms of memory.

Just my 2c. :-) Happy to answer questions.

[markshannon]

@ntoll
@dpgeorge and I both agree that PULL_UP, PULL_DOWN and NO_PULL should be defined, we differ in what values they should hold.
The difference is what should this print:

pin0.set_pull(pin0.PULL_DOWN)
pin1.set_pull(pin1.NO_PULL)
pin2.set_pull(pin2.PULL_UP)
print(pin0.get_pull(), pin1.get_pull(), pin2.get_pull())

I think it should be -1, 0, 1
@dpgeorge suggests the underlying Nordic values, which would be 1, 0, 3 (and would save a few bytes of flash).
If I understand correctly, @deshipu would be happy with either -1, None, 1 or 1, None, 3

It looks like @ntoll has the deciding vote. Choose wisely 😄

[dpgeorge]

The difference is what should this print ...

Not exactly. What I'm saying is that the return value of pin.get_pull() is an opaque value and has no direct meaning to the user (they should not rely on it being anything sensible, it could even change from one version to the next). So it makes no sense to print it out. Instead, the things you can do with the return value are: 1) save it to a variable, 2) use it as an argument to pin.set_pull(), 3) compare it with the constants PULL_UP, PULL_DOWN, NO_PULL.

The first question is: should pin.get_pull() return an opaque or transparent (ie user can take meaning from it) value? I vote opaque.

If it's transparent, then the follow-up question is: what values should these transparent values be?

[markshannon]

I think we agree that the documentation should recommend that the user treats the value as opaque,
and I agree that the user should treat the value as opaque. But what does "opaque" really mean?

It must have some value, and that value will be observable.
You simply cannot prevent people from printing it out.
In fact, when using the repl I like to be able to inspect the state of the peripherals.

[ntoll]

What should the values be? I believe they should be obvious and unsurprising.

Is there a precedent in this case?

Are hardware engineers expecting the Nordic register values? It strikes me this may be a reasonable expectation.

Alternatively, is there an existing numeric representation of pull up, pull down or no pull? If there isn't we should be careful: what value do we add by making one? We may even confuse people with a non-standard convention.

On balance, if we use Nordic register value we can flag what this means in our documentation, we save a small amount of memory and we give learners an opportunity to ask, what exactly is a register value?

Hope this makes (careful) sense. ;-)

[dpgeorge]

But what does "opaque" really mean?

In this context, to me opaque means that it's not defined in the docs to be any particular value, the user should not rely on it being any particular value, and it can change at any notice and user code should not break.

As such we're free to use any values, and using nordic register values is the simplest and most efficient (in code size and code execution).

[markshannon]

OK, I've changed the pull constants to be the nordic provided values.
The tests still pass.

[markshannon]

I've tacked the changes to the pull values to the final commit, so this PR should be squashed when merging.

[dpgeorge]

OK, I've changed the pull constants to be the nordic provided values.

I don't see the changes...?

this PR should be squashed when merging.

GtiHub only allows "squash all and merge" or "rebase and merge" (we want the latter). So If you want to have it squashed please do it on your branch corresponding to this PR.

[markshannon]

Rebased, squashed and retested.

[dpgeorge]

It's done!