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

Code cleanup #4509

Closed
wants to merge 5 commits into from
Closed

Code cleanup #4509

wants to merge 5 commits into from

Conversation

fabianoms
Copy link
Contributor

FLASH_SECTOR_SIZE defined in terms of SPI_FLASH_SEC_SIZE (issue #2421).

FLASH_SECTOR_SIZE defined in terms of SPI_FLASH_SEC_SIZE (issue esp8266#2421).
This was a necessary include so that SPI_FLASH_SEC_SIZE can be referenced.
@devyte
Copy link
Collaborator

devyte commented Mar 16, 2018

@fabianoms Thanks, but looks like something is still missing, because CI didn't build. Can you figure it out?

@fabianoms
Copy link
Contributor Author

@devyte just one check failing now. Looks like the host tests don't reference files from "tools/sdk/include/". I'm not sure if I forgot to add something in flash_utils.h or I have to edit the "Arduino/tests/host" to add some reference to sdk files.

@devyte
Copy link
Collaborator

devyte commented Mar 20, 2018

@fabianoms I was just looking at the same. Honestly, I'm not sure what's the best way to go here.
@igrr would it be correct to copy c_types.h from the sdk include dir into the host/common dir, and edit as necessary, similar to how it's done with Arduino.h?

@igrr
Copy link
Member

igrr commented Mar 20, 2018

TBH, i would much rather not pull the stuff defined in c_types.h into the global namespace. Easiest is probably replace all uint32 with uint32_t in spi_flash.h. But then, spi_flash.h also declares a bunch of functions. Do we need them in the global namespace, just for the flash sector size definition? I'm not sure.

@devyte devyte self-assigned this Mar 23, 2018
@devyte
Copy link
Collaborator

devyte commented Nov 9, 2018

Closing in favor of #5327 .

@devyte devyte closed this Nov 9, 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.

3 participants