Closed fivdi closed 3 years ago
@fivdi - You raise good points.
It is impractical to set more than 32-bits as a single value. I think it could be done with BigInt
but that seems extreme for a constrained device. (You could use an array of integers instead of a number, but that has its own overhead.)
So, yes, a host needs to decide the best way to divide its GPIO. As you note, even on a device with under 32 GPIO pins there might be a valid reason to divide those.
The standard could offer guidance. For example, when there are multiple banks, that the bank is selected by a property named bank
. The value would be host specific (both a number or string could make sense, depending on the host).
I'll look into some light edits.
Please take a look at the commit above.
Shouldn't bank
be a property on a pin specifier object instead of its own property? It seems analogous to the port
property in this example
But there is no pin specifier because the pins are implicitly specified by the bank. Or maybe I am misunderstanding your point?
You are correct that the bank
property is analogous to port
. I considered using port
here, but thought bank
was more clear. If there is a strong sense that port
would be better, I'm happy to make that change.
I should have written "pins", not "pin" in my previous comment.
I don't have a strong feeling about the name and allowing an object for pins
just like we do for pin
gives implementors the flexibility to use a name that matches their own hardware conventions.
Thanks, that helps. I think I understand now.
The value of the pins
property is defined as a bitmask, not a pin specifier. Allowing it to be an object to also specify the bank/port feels like a misleading parallel.
Assuming the use of port
, calling code would look like this:
new DigitalBank({
pins: 0x00ff,
port: 1,
...
});
That is concise and maintains pins
and port
together at the same level of the hierarchy as in the pin example you cited (expanded here for emphasis):
new Serial({
transmit: {
pin: 5,
port: 1
},
receive: {
pin: 6,
port: 2
},
...
}
I think what you are proposing is this? It adds a level to object description that isn't functionary but does require a new property name.
new DigitalBank({
pins: {
bitmask: 0x00ff, // or name this property pins, but that is strangely redundant
port: 1
}
...
});
Please take a look at the commit above.
I think it's good and much clear now.
Additional comments:
bank
property uses the term "IO Sources" which is a new term in the specification so the reader will need to decide what an IO source is. I initially decided that an IO source is an MCU but I'm having second thoughts. I now think that an IO source could be more than an MCU, for example, it could be an MCU plus a GPIO expander if the system had a GPIO expander. Of course, I'm speculating here.DigitalBank
class has a constructor property called bank
because there are IO sources with more than a single digital bank and this property is explained in the specification, it makes me wonder why the Serial
class doesn't have a similar property called port
that's explained in the specification because there are IO sources with more than a single serial port. As mentioned here, a port
property would also be possible for the Serial
class.Per
The value of a pin specifier is host-dependent. It is often a number corresponding to the logical GPIO pin number as per the hardware data sheet (e.g. GPIO 5), but it may be a string ("D1") or even an object ({port: 1, pin: 5}).
We would likely see:
new Digital({
pin: {
pin: 5,
port: 1
},
...
});
So the redundancy has precedent. However, the property names in a pin specifier object are completely dependent on the implementor/provider so we don't have to prescribe what those should be.
If we allow the implementor to add properties like bank
or port
directly onto the Digital Bank IO options object (which seems fine) then we should treat the other IO types the same way. That means the previous example would simply read:
new Digital({
pin: 5,
port: 1
...
});
@dtex - I don't believe the second form is correct. If the port is necessary to specify which pin you are connecting to, then it needs to be part of the pin specifier value. The standard requires information related to specifying a pin be part part of the pin specifier. To do otherwise would create incompatibles with how the host provider instance is specified. See the Pin Name Property section for an example of where this breaks down.
Further, the second form would create an inconsistency in how pins are specified. While it could work for any IO class that has a single pin, it does not for those with multiple pins.
The Digital Bank is the outlier here because it does not use pin specifiers. Consequently, it should not be used a model for those that do. The reason it does not use pin specifiers is because what it exists to provide optimized bulk access to access certain pins.
If the port is necessary to specify which pin you are connecting to, it needs to be part of the pin specifier value
This makes perfect sense, and I agree that's the way it should be. I'm totally on board with the first two paragraphs of your comment.
The Digital Bank is the outlier here because it does not use pin specifiers
Here's where I'm getting hung up.
While the constructor's bitmask may not match the specification's definition of a "pin specifier" it's hard to not think of it as a pin specifier since it is being used to specify which pins are included in the bank. Following that reasoning, if someone has learned from using the other IO classes that a Pin Specifier can be an object that includes port
, bank
(or some other hardware-dependent pin grouping) then they would probably assume that the same is true for DigitalBank... I know I would.
... it's hard to not think of it as a pin specifier since it is being used to specify which pins are included in the bank
It is a bitmask, not a pin specifier. If a developer does not understand that, their code will fail and their incorrect assumption will be rapidly be apparent. ;)
Sorry, I am sympathetic to the challenges developers face when learning a new API. But, I cannot imagine introducing an inconsistency into the API in order to allow a potential misunderstanding to work.
Well, that handles my comments, but I see that I've kinda hijacked this issue. We should come back to @fivdi's additional comments here
@fivdi - apologies for the delay. Let's get back to these.
The description of the bank property uses the term "IO Sources" which is a new term in the specification so the reader will need to decide what an IO source is. I initially decided that an IO source is an MCU but I'm having second thoughts. I now think that an IO source could be more than an MCU, for example, it could be an MCU plus a GPIO expander if the system had a GPIO expander. Of course, I'm speculating here.
Yes, when drafting the text I realized that "IO Source" was a new term. You are correct that it does not refer exclusively to a MCU, as the IO could be from an IO Provider like a GPIO expander or a remote source of IO (Firmata over TCP, for example).
Rewording the text to avoid introducing a new term could be the easiest solution: "For implementations with more than a single digital bank...". I've done that. Improvements are welcome.
The abbreviation MCU is only used once in the specification. It's explained in the Terms and definitions section. I guess this single occurrence could be removed as it doesn't really make sense to define a term that is never used.
Fair point. I'll comment that out.
If the DigitalBank class has a constructor property called bank because there are IO sources with more than a single digital bank and this property is explained in the specification, it makes me wonder why the Serial class doesn't have a similar property called port that's explained in the specification because there are IO sources with more than a single serial port. As mentioned here, a port property would also be possible for the Serial class.
I'm fine with formally defining port
, as we defined bank
here. I just want to be certain we are talking about the same thing.
port
for the driver name (/dev/cu.0001
).Rewording the text to avoid introducing a new term could be the easiest solution: "For implementations with more than a single digital bank...". I've done that. Improvements are welcome.
The wording is much better now.
I'm fine with formally defining
port
, as we definedbank
here. I just want to be certain we are talking about the same thing....
Here you describe what a serial port is in various different contexts. I'll have to admit that I wasn't thinking of all these different contexts. I was thinking of what MCU datasheets often refer to as peripherals. For example, Table 5: Memory and Peripheral Mapping
on page 17 of the ESP32 Series Datasheet lists peripherals called UART0, UART1 and UART2. The table also lists multiple SPI and I2C peripherals. These are the "ports" that I had in mind. If I'm not mistaken, for a UART, the ESP32 allows any pin to be used for transmit or receive.
I wasn't thinking of ports on computers running operating systems like macOS or Linux. I somehow assumed that this specification was specifically for MCUs rather than also being for MPUs or application processors typically found on a Mac or Linux machine.
If port is formally defining for Serial then I think it should also be formally defined for I2C and SPI.
I'll have to admit that I wasn't thinking of all these different contexts.
Yea, they didn't all come to me at once either. ;) I wanted to explore the possibilities to help inform the design.
I was thinking of what MCU datasheets often refer to as peripherals
That's what I understood you had in mind. My second bullet point was my attempt to describe that.
If I'm not mistaken, for a UART, the ESP32 allows any pin to be used for transmit or receive.
That may be true for UART. I know for SPI on ESP32 that there are performance differences depending on which pins the SPI peripheral is routed to (the reference to "functionally equivalent" above).
I wasn't thinking of ports on computers running operating systems like macOS or Linux. I somehow assumed that this specification was specifically for MC
The design priority of the specification is MCUs. But that doesn't automatically preclude it from working elsewhere. For example, we have recently had good success using the Serial IO API on computers. That could be meaningful for a simulator or just portability. But, we should not make design choices that favor a computer at significant expense to MCUs
If port is formally defining for Serial then I think it should also be formally defined for I2C and SPI.
Agreed. I'll give that try and let you know when a revision is available.
I pushed edits to address the ports discussion. The cleanest solution seemed to be to define port specifier in the same spirit as pin specifier. That can then be concisely referenced from the IO types that use it. The definition is a little rough. Suggestions for improving it are welcome.
The modifications in commit ec38ffe7aa3c496748cb1cdcf841fc1a4b1bc0ac look good to me. Given that there are potentially many different types of port, the definition of port is likely to remain a little rough.
Given that there are potentially many different types of port, the definition of port is likely to remain a little rough.
Good point. Hardware is like that...
Thanks for the review and your feedback. Both are very much appreciated.
I have a few questions related to the DigitalBank class.
Edit Assuming the intent of DigitalBank is to enable efficient access to the values of multiple GPIO pins of the same GPIO port concurrently, it may, depending on the MCU, make sense for a DigitalBank to allow access to 16 GPIOs rather than 32 as the GPIO ports on some MCUs have up to 16 I/Os rather than up to 32.