esp8266, stmhal, zephyr: Rename machine.Pin high/low methods to on/off. #3025
Conversation
|
This change would conclude finalization of machine.Pin and machine.Signal interface, i.e. the most basic level of the MicroPython HW API. Without this change, the potential of the Signal class comes not fully realized. The basic requirement for the introduction of Signal class was that it's fully duck-type compatible with Pin, and the following usecase can be realized: "If someone (simple-mindedly) developed an application using Pin and assuming a particular polarity for that Pin, it should be possible to make it compatible with devices using different polarity by just replacing Pin object declaration with a Signal declaration, without changing the rest of the program logic." |
|
This will break too many things for pyboard (and probably also a lot for esp8266). In stmhal, machine.Pin and pyb.Pin have the same underlying implementation and pyb.Pin will continue to have high/low. If you run
That's already achievable just by using pin.value([x]), with no mention of on/off/high/low. |
Sounds good for pyb.Pin to continue to have high/low, even if it leaks into stmhal's Pin.
Well, then we need to change them to make this saga over.
Then it's a usecase for using Signal.
Right, but we wanted to go a step further than .value(), and add convenience methods e.g. to turn on LED on and off. We actually did that, and added methods .high() and .low(). But later it became clear that the naming isn't right for all cases. But we'd like to keep functionality (spirit of the API), even if we need to make backward incompatible changes to method names. The Signal is intended to be a simple, but powerful addition to Hardware API, but of course it brings some compromises to it. Even if it's a compromise, it comes as a natural next (also, final) step on this way. And it's similar to refactorings done to I2C and SPI classes - they both required incompatible changes, but now we have much better APIs for them. |
|
Reinstated pyb.Pin.high/low for stmhal. Updated drivers/* to use .value(1)/.value(0) instead of .high()/.low() (didn't go to add Signal dependency there for now). This patch should be ready to merge now. |
|
@dpgeorge : ping |
|
I'm not happy how all this pin/signal stuff has turned out. I think that high()/low() methods are useful for pins and should be kept as part of the Pin API. For example, if I create a CS (chip select) pin then it's conceptually an entity which goes low to enable it, and so writing The issue is that duck typing is overly used in this situation, that on/off are being overused for too many concepts. The solution is to keep high()/low() methods strictly for Pin only, and they should be used when you really mean that you want the output to be vdd/gnd. And then there would be an error if you passed in a Signal object. And then on()/off() can be implemented for both Pin and Signal, and are used when you conceptually want to turn something on or off (eg an LED, whose implementation as active low or high may change). |
The set of answers is clear:
Accidentally sounds good. Well, that's the nature of Pins vs Signals - they're supposed to be fully replaceable. Yes, it brings some entailments regarding "accidentally". But someone can remove the main code of the application accidentally, then there're no errors, it just behaves strangely.
Those things are like atoms - all the differently looking things consist of the same atoms, so it's easy to get confused, so one should take care not to.
Well, that's pretty good compromise for something started with "I'm not happy how all this pin/signal stuff has turned out.". That still doesn't adhere to the original requirements I started with (the original idea was to add inversion capabilities to Pin; that led to unneeded bloat on Pin level, so the idea became to split out inversion capability to a separate class, but naturally, Pin and this new class should be able to fully replace each other). It also has a problem from the point of the minimality of the API. So, for consistency, I still think that design where Pin and Signal are fully replaceable and maintain the minimal API (no high()/low()) is better. But well, it's only so much of the arguments I have. It's hard to predict real-world usage fully, again, I think we can get over it, the principle of the minimal API will be our guiding star, but well... . So, please tell what should be done. |
@dpgeorge on/off doesn't make sense for many uses of a digital pin or an inverted pin. It makes sense to have high/low in Signal (see use cases below). I agree with @pfalcon that Pin and Signal should be interchangeable, which means both should support on/off and high/low. Pins are often used as a selector, not just as an on/off switch. Examples are LCD displays have one address pin called CMD/DAT. High for command, low for data. 2 channel ADC's have an S pin to select channel. High for ch1, low for ch0. Garage door. High for up, low for down. In these cases there are only two on states, and no off state. The data sheet will be written on terms of 1/0 or high/low. In some cases the user will need to drive that pin through an inverter so using Signal inverted makes sense. High/low represent the signal level at the device. A real world example: An automatic milk shake machine needs a 5V signal to select flavour. The data sheet says high for Strawberry and low for Chocolate. I use an open drain pin with a 5V pullup. I use a Pin (or Signal non inverted) constructor and flavour.high() for Strawberry. After building a thousand we find that occasionally in hot whether the pullup isn't strong enough and the machine won't make Strawberry milkshakes. I decide to use a spare 74ACT04 gate as an inverting level translator (the others are driving blue LEDs so I had a free gate left over). I change the constructor to use Signal inverted and the method all through the program doesn't change. flavour.high() is still Strawberry. Using flavour.on() does not make sense for Strawberry. The good thing about keeping Pin and Signal fully compatible is you only need to change the constructor in one place to accommodate hardware changes. You never have to change the methods throughout your program. I will restate my previous comment on the name "signal". Signal is confusing. It has many other meanings, probably on the same system that is using Signal for DigitalIO. Audio signal, vibration signal, WiFi signal strength, etc. It would be much less confusing for users to keep Pin as the standard function for application level pin waggling, including the invert feature, and provide a fast pin class for libraries and users who need the speed for serial busses, or lower memory usage.
This means users don't need to be encouraged to change to Signal. Most users won't notice any difference and can slowly clean up their code to use inverted where applicable. Those that hit problems with speed or memory are probably more advanced users and will understand to switch to Pin_fast. I don't see a downside to this - less impact on most users. On the subject of names pin.value(x) does not match the actual hardware of most microcontrollers. Usually you write to the output data register (ODR) and sometime this changes the pin state. A better syntax would be pin.out_value(x) as the complement to pin.out_value(). However pin.value() is probably too entrenched to change now. |
|
Initial doc for Signal was pushed: 0ba136f |
|
Ok, going by the principle of "micro" to keep things minimal, and by Python's principle of "there should be only one way to do something" let us indeed remove high()/low() from the Pin API (except for pyb.Pin which retains them for legacy reasons). @pfalcon I would prefer to use the short-hand Finally: if Signal is to really replicate Pin then it's going to need the irq() method, and it's going to need to invert the IRQ_RISING/IRQ_FALLING levels for the interrupt. |
|
Thanks! I think it's good approach "simple things (e.g. Pin) should be simple, and more complex (more advanced blocks) can be more elaborated". I'm preparing to travel, so it make take into a weekend to update this patch and merge it.
Yes, per the previous discussion, there was no talk about removing it, as .value() is implemented in terms of
Well, I'm not sure about this. The idea was that Signal is fully portable across ports, and being able to trigger IRQ is by definition machine-specific. Of related note, I come to conclusion that machine.time_pulse_us() should not work with Signal (not a usecase for time_pulse_us() which is intended to be able to time real-time signals), so https://github.com/micropython/micropython/blob/master/examples/hwapi/button_reaction.py#L14 will need to rewritten. All in all, this patch finishes defining the core interface of Pin and Signal. And we can start elaborating and fully defining interface for IRQ, wake, sleep config (like recent report of being able to configure sleep Pin levels for esp8266, the same problem should be with esp32) - all that in connection with soft irq aka .schedule(). Once that's done, we can see what of that can "leak thru" to Signal. |
For consistent Pin/Signal class hierarchy. With it, Signal is a proper (while still ducktyped) subclass of a Pin, and any (direct) usage of Pin can be replace with Signal. As stmhal's class is reused both as machine.Pin and legacy pyb.Pin, high/low methods actually retained there.
|
All changes are done, the patch is ready for merge. |
|
Merged. |
|
Ok, thanks for doing this. |
STM32: Add DTCM and ITCM support to F7 series
For consistent Pin/Signal class hierarchy. With it, Signal is a proper
(while still ducktyped) subclass of a Pin, and any (direct) usage of Pin
can be replace with Signal.