Skip to content
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.

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

stmhal/dma.c: Modify dma_init() to accept init params as a struct #1344

Closed
wants to merge 1 commit into from

Conversation

blmorris
Copy link
Contributor

This removes hard-coded DMA init params from dma_init(), instead requiring
that the params are set when the dma handle is created. passed as an argument to dma_init()
This allows dma_init to be more generic so it can be used in cases like
I2S and SD Card which require different initialization parameters.

Edited to reflect changes to the commit.

@blmorris
Copy link
Contributor Author

This commit appears to work correctly, but I am going to keep banging on it to make sure it didn't break SPI or I2C functions. If this looks like the wrong way to solve the problem, I'm happy to accept that ;)
The problem that I'm trying to solve is discussed here: 3d30d60#commitcomment-11771235

@blmorris
Copy link
Contributor Author

Please disregard this PR for now - I mixed up my firmware images and was testing on the master branch. This commit hard-faults on SPI transfers. I will attempt another fix.

@blmorris
Copy link
Contributor Author

This seems to work now - I squashed down to a single commit because the many of the changes in this fix just reversed things from the original PR.
One possible improvement - i2c_dma_init and spi_dma_init are identical, so it may make sense to declare a single structure in dma.h and call it spi_i2c_dma_init.

@blmorris blmorris changed the title stmhal/dma.c: Modify dma_init() to accept init params as part of handle. stmhal/dma.c: Modify dma_init() to accept init params as a struct Jun 22, 2015
@@ -80,7 +80,7 @@ static int get_dma_id(DMA_Stream_TypeDef *dma_stream) {
}
}

void dma_init(DMA_HandleTypeDef *dma, DMA_Stream_TypeDef *dma_stream, uint32_t dma_channel, uint32_t direction, void *data) {
void dma_init(DMA_HandleTypeDef *dma, DMA_Stream_TypeDef *dma_stream, DMA_InitTypeDef dma_init, uint32_t dma_channel, uint32_t direction, void *data) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should take a pointer to the DMA_InitTypeDef to avoid copying a large amount of data for the function call.

@dpgeorge
Copy link
Member

One possible improvement - i2c_dma_init and spi_dma_init are identical, so it may make sense to declare a single structure in dma.h and call it spi_i2c_dma_init.

Yes I agree it should be done this way. Maybe call it dma_init_struct_spi_i2c.

@blmorris
Copy link
Contributor Author

Great, thanks for the feedback. All makes sense to me, I'll update the PR tonight.

@blmorris
Copy link
Contributor Author

Updated PR to incorporate suggested changes, but I haven't been able to build or test yet.
Currently the linker is failing with the following error:

CC dma.c
CC i2c.c
CC spi.c
LINK build-PYBV10/firmware.elf
build-PYBV10/i2c.o:(.data.dma_init_struct_spi_i2c+0x0): multiple definition of `dma_init_struct_spi_i2c'
build-PYBV10/dma.o:(.data.dma_init_struct_spi_i2c+0x0): first defined here
build-PYBV10/spi.o:(.data.dma_init_struct_spi_i2c+0x0): multiple definition of `dma_init_struct_spi_i2c'
build-PYBV10/dma.o:(.data.dma_init_struct_spi_i2c+0x0): first defined here
make: *** [build-PYBV10/firmware.elf] Error 1

I think I know what the problem is (defining a structure in dma.h which is then included in dma.c, i2c.c and spi.c) but I'm not sure the best way to resolve it.

@dpgeorge
Copy link
Member

You need to declare the struct in dma.h using extern and then define it in dma.c.

@blmorris
Copy link
Contributor Author

@dpgeorge - Thanks, I had just figured it out myself and pushed an update, but I somehow thought the dma init struct needed to be defined in either spi.c or i2c.c to avoid a 'variable declared but not used' error.
I put the shared init struct into dma.c as you suggested and it worked. I amended the commit to do it that way.
It's not a perfect match to your feedback - in dma.c I needed to use dma->Init = *dma_init; instead of *dma->Init = *dma_init; to get it to compile; it seems to work, hopefully it's actually correct.

@dpgeorge
Copy link
Member

in dma.c I needed to use dma->Init = *dma_init; instead of *dma->Init = *dma_init; to get it to compile; it seems to work, hopefully it's actually correct.

Yes what you did is correct.

@blmorris
Copy link
Contributor Author

Yes what you did is correct.

Great, thanks. Would it help if I squashed it all down to one commit?

@dpgeorge
Copy link
Member

You can squash, but I'll probably need to rebase anyway so will do it then, no problems.

This removes hard-coded DMA init params from dma_init(), instead defining
these parameters in a DMA_InitTypeDef struct that gets passed as an
argument to dma_init()
This makes dma_init more generic so it can be used for I2S and SD Card,
which require different initialization parameters.
@dpgeorge
Copy link
Member

Merged in c517552. Note that I made the dma_init_struct_spi_i2c const so that it's stored in ROM.

@dpgeorge dpgeorge closed this Jun 24, 2015
@blmorris
Copy link
Contributor Author

Merged in c517552. Note that I made the dma_init_struct_spi_i2c const so that it's stored in ROM.

Good. I meant to do that but didn't figure out how - I now see that the const keyword is needed in every declaration.

@blmorris blmorris deleted the dma-init branch June 24, 2015 17:33
nickzoic pushed a commit to nickzoic/micropython that referenced this pull request Dec 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants