8000 stmhal: Add I2S support to make-pins.py by dhylands · Pull Request #1362 · micropython/micropython · GitHub
[go: up one dir, main page]
Skip to content

stmhal: Add I2S support to make-pins.py #1362

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

stmhal: Add I2S support to make-pins.py #1362

wants to merge 1 commit into from

Conversation

dhylands
Copy link
Contributor
@dhylands dhylands commented Jul 3, 2015

This adds support for I2S pins (which adds 400 bytes to the image when I2S is enabled).

I also introduced the notion of conditional pins. For example, if MICROPY_HW_ENABLE_SPI3 is set to 0, then it won't define the alternate functions for SPI3.

The addition of conditional pins drops 264 bytes off the pyboard image (without I2S), and saves 92 bytes on the ESPRUINO_PICO image.

@dhylands dhylands force-pushed the stmhal-make-pins-i2s branch from 0880bb0 to d83d5a1 Compare July 5, 2015 16:53
@dpgeorge
Copy link
Member
dpgeorge commented Jul 6, 2015

Looks okay, just 2 points:

  1. Can we use "EXTSD" instead of "extSD" to be consistent with capitalisation? Better yet, is there a more standardised name for this second data pin, like SD2? If we were on a different manufacturers MCU then what would it be called?
  2. It's a bit inconsistent at the moment how some things are enabled by specifying their pin/port (eg I2C, UART), while other things are enabled by a direct "ENABLE" macro (like SPI, CAN). I don't know the right solution to this and probably it's best left for a patch later on.

@blmorris
Copy link
Contributor
blmorris commented Jul 6, 2015

@dpgeorge - The naming scheme for I2S stuff is a bit funny and inconsistent, which I think is a result of I2S not really being a first-class peripheral in the STM32F architecture. Specifically, in simplex (one-directional) operation, I2S is just a special configuration of SPI, and the HAL just uses SPIx rather than I2Sx as the instance name. For duplex operation, the hardware provides a second stripped-down SPI instance which can only be configured as a slave-mode I2S peripheral; this instance is named I2S(x)_ext in the HAL.
I would support changing the names and capitalization from what is used in the HAL for consistency and readability - but I don't know that we can be consistent with another architecture which implements I2S as a first-class peripheral (Blackfin is one architecture I am aware of that does this, and their I2S implementations can support 4 or more stereo data lines per instance)

@dhylands
Copy link
Contributor Author
dhylands commented Jul 6, 2015

Looks okay, just 2 points:

Can we use "EXTSD" instead of "extSD" to be consistent with capitalisation? Better yet, is there a more standardised name for this second data pin, like SD2? If we were on a different manufacturers MCU then what would it be called?

TI seems to call the pins DX and RX rather than SD and extSD. I can change the AF's to be uppercase. I couldn't find any other examples, but I'm not realy familiar with i2S.

It's a bit inconsistent at the moment how some things are enabled by specifying their pin/port (eg I2C, UART), while other things are enabled by a direct "ENABLE" macro (like SPI, CAN). I don't know the right solution to this and probably it's best left for a patch later on.

Yeah - I was just going with the flow on this count. If we decide to change thing, it feels more logical to do it as a separate patch.

@dhylands dhylands force-pushed the stmhal-make-pins-i2s branch from d83d5a1 to cda6743 Compare July 6, 2015 16:43
@dhylands
Copy link
Contributor Author
dhylands commented Jul 6, 2015

I changed extSD to EXTSD

@blmorris blmorris mentioned this pull request Jul 6, 2015
Closed
@dpgeorge
Copy link
Member
dpgeorge commented Jul 6, 2015

Ok, there is a problem: the constants MICROPY_HW_ENABLE_I2S2 and MICROPY_HW_ENABLE_I2S3 are not defined in any mpconfigboard.h files and are used as #if MICROPY_HW_ENABLE_I2S2. I'd rather not have to add these definitions to all mpconfigboard.h files (all defined to 0). Instead we should decide on a config variable that can be used in #if defined(MICROPY_HW_I2S...).

@blmorris are there any configuration values that are needed for I2S, or is it simply that it's enabled/disabled for a given board? If so we could just use MICROPY_HW_I2S2 and MICROPY_HW_I2S3, and only define them for boards that want these peripherals enabled.

For convenience in your i2s.c file you can optionally define MICROPY_HW_ENABLE_I2S, so it doesn't require too many changes.

@blmorris
Copy link
Contributor
blmorris commented Jul 6, 2015

I do have the two values added together in mpconfigport.h to determine how many slots to reserve in the root pointer array, but I can find another way to do that.

@dhylands
Copy link
Contributor Author
dhylands commented Jul 6, 2015

Ok, there is a problem: the constants MICROPY_HW_ENABLE_I2S2 and MICROPY_HW_ENABLE_I2S3 are not defined in any mpconfigboard.h files and are used as #if MICROPY_HW_ENABLE_I2S2. I'd rather not have to add these definitions to all mpconfigboard.h files (all defined to 0). Instead we should decide on a config variable that can be used in #if defined(MICROPY_HW_I2S...).

Hmm. I was relying on the fact that you can use:

#if FOBAR

and if FOOBAR isn't defined then its assumed to be zero. Or do we have some warning or something that's preventing that? I didn't run into any problems locally.

Otherwise, I could have make-pins.py just wrap those like this:

#if defined(FOOBAR)
#if FOOBAR
  AF(...)
#endif
#endif

@dhylands
Copy link
Contributor Author
dhylands commented Jul 6, 2015

GCC explicitly documents #if FOO where FOO is not defined at being legal:
https://gcc.gnu.org/onlinedocs/cpp/If.html (see the last bullet)

It looks like an explicit -Wundef would cause grief, but I'm not seeing that used anywhere.

@blmorris
Copy link
Contributor
blmorris commented Jul 6, 2015

I'm just as happy to assume that MICROPY_HW_ENABLE_I2S2 and MICROPY_HW_ENABLE_I2S3
are either defined or not, rather than assuming that they are defined to be 0 or 1; I'll figure out a way to adapt my code to do it that way.
In the meantime, I would rather not have this PR held up by that detail, since I will be doing a pretty substantial rework of the I2S code anyway based on this.

@dpgeorge
Copy link
Member
dpgeorge commented Jul 6, 2015

@dhylands you are right that it compiles ok as-is, but I'd rather keep it clean to prevent any unforseen errors.

As you say, it's easy to just make it #if defined(X) && X; that would be a good fix for now.

@dhylands dhylands force-pushed the stmhal-make-pins-i2s branch from cda6743 to e15ce61 Compare July 6, 2015 23:33
@dhylands
Copy link
Contributor Author
dhylands commented Jul 6, 2015

ok - updated make-pins.py to output

#if defined(MICROPY_ENABLE_FOOBAR) && MICROPY_ENABLE_FOOBAR

if MICROPY_ENABLE_FOOBAR contains the word ENABLE, otherwise if just outputs:

#if defined(MICROPY_FOOBAR_SOMETHING_ELSE)

if MICROPY_FOOBAR_SOMETHING_ELSE doesn't contain the work ENABLE

@blmorris So this means that you should be able to leave MICROPY_HW_ENABLE_I2S2 #defined to a (0) or (1) and we don't need to define it everywhere else. The SPI macros already behave the same way.

@dpgeorge
Copy link
Member
dpgeorge commented Jul 7, 2015

Thanks! Merged in 11115e4.

@dpgeorge dpgeorge closed this Jul 7, 2015
@blmorris
Copy link
Contributor
blmorris commented Jul 7, 2015

@dhylands - Thanks for this!
One question - should I assume for now that we won't make all of the PIN_TYPE_xxx values unique like you suggested in #1361 (comment) ?
It's fine with me either way, I'll start with this and if we decide to make the PIN_TYPE values unique down the road its an easy change.

@dhylands
Copy link
Contributor Author
dhylands commented Jul 7, 2015

I think it makes sense to make the PIN_TYPE_xxx values unique as part of your patch, since this is where the function to use them would be introduced.

@dpgeorge
Copy link
Member
dpgeorge commented Jul 7, 2015

I think it makes sense to make the PIN_TYPE_xxx values unique as part of your patch, since this is where the function to use them would be introduced.

But at least make them a separate commit.

@blmorris
Copy link
Contributor
blmorris commented Jul 7, 2015

But at least make them a separate commit.

How about I make a separate PR that also adds a new pin_find_type() function to pin_named_pins.c?
While this feature is motivated by my I2S development, it seems to me that it is distinct enough that it shouldn't be buried in my I2S patch. Also, I suspect that my I2S patch is still going to need a bit of work before it's ready for merging ;)

@dpgeorge
Copy link
Member
dpgeorge commented Jul 7, 2015

How about I make a separate PR that also adds a new pin_find_type() function to pin_named_pins.c?

If you need such a function for I2S then, yes, that sounds good.

@blmorris
Copy link
Contributor
blmorris commented Jul 7, 2015

If you need such a function for I2S then, yes, that sounds good.

Yes, though ultimately it could offer a clean way for other peripherals to do what I am proposing for I2S, which is to take a list of pin names and configure the peripheral to use those pins after confirming that they will work. (You have a comment in spi.c to the effect of wanting to be able to do this, though I don't know if it is still important to you.)

@dhylands dhylands deleted the stmhal-make-pins-i2s branch November 8, 2015 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dhylands @dpgeorge @blmorris
0