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

cleanup/unify flash sector size define value #5327

Merged
merged 15 commits into from
Nov 24, 2018
Merged

cleanup/unify flash sector size define value #5327

merged 15 commits into from
Nov 24, 2018

Conversation

devyte
Copy link
Collaborator

@devyte devyte commented Nov 9, 2018

Fixes #2421
Replaces #4509

@devyte devyte requested a review from igrr November 9, 2018 18:27
@devyte devyte mentioned this pull request Nov 9, 2018
@devyte
Copy link
Collaborator Author

devyte commented Nov 16, 2018

Further rework is possible to unify eboot defines as well, but that should be done separately.

@devyte devyte requested a review from d-a-v November 16, 2018 16:13
@devyte
Copy link
Collaborator Author

devyte commented Nov 16, 2018

@d-a-v you've dabbled with sdk updates. Does this factoring out make sense? I'm worried about migrating to a newer sdk. Maybe this should be a patch or something?

Copy link
Collaborator

@d-a-v d-a-v left a comment

Choose a reason for hiding this comment

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

Per this comment

/* Definitions are placed in eboot. Include them here rather than duplicate them.
 * Also, prefer to have eboot standalone as much as possible and have the core depend on it
 * rather than have eboot depend on the core.
 */

then should spi_flash_geometry.h go to eboot ?

@devyte
Copy link
Collaborator Author

devyte commented Nov 21, 2018

There are three code pieces involved: eboot, the sdk, and our core, and all three had duplicate definitions in one way or the other. The sdk comes from Espressif and could be updated, eboot is hosted by us and is not included in the sdk, and our core is our own.
My thinking was:

  • eboot was dependent only on itself only through code duplication. It's ok to have it depend on the sdk, but must reduce external dependencies when at all possible.
  • core depends on sdk and eboot
  • sdk depends only on itself

From the above, the flash geometry must be in the sdk. If I were to put them in eboot, the sdk would depend on eboot, which we don't want.

@devyte
Copy link
Collaborator Author

devyte commented Nov 22, 2018

Any further comments? Feel free to destroy my thinking above.
I'm still worried about maintaining this change across sdk updates.

@devyte
Copy link
Collaborator Author

devyte commented Nov 24, 2018

Given that there are no further comments, and in the interest of moving forward, I'm merging this as-is. Any issues that arise from updating the sdk in the future will be handled at that time.

@devyte devyte merged commit 3d70f43 into esp8266:master Nov 24, 2018
@devyte devyte deleted the issue2421 branch November 24, 2018 05:59
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.

2 participants