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

Implement get_board_f_image #1308

Merged
merged 5 commits into from
Feb 15, 2024
Merged

Implement get_board_f_image #1308

merged 5 commits into from
Feb 15, 2024

Conversation

Jason2866
Copy link
Contributor

@Jason2866 Jason2866 commented Feb 13, 2024

needed since the build recipe in Arduino has changed. https://github.com/espressif/arduino-esp32/blob/master/platform.txt#L159

Related PR espressif/arduino-esp32#9243

@valeros
Copy link
Member

valeros commented Feb 14, 2024

Hi @Jason2866, thanks for the PR. I guess this new option still defaults to the flash frequency? Does it make sense to reuse already available functionality as a fallback value? Something like this:

def _get_board_img_freq(env):
    return env.BoardConfig().get("build.img_freq", _get_board_f_flash(env))

@Jason2866
Copy link
Contributor Author

Jason2866 commented Feb 14, 2024

@valeros yes, the suggestion from you should do too. Thx. Changed.

EDIT: Isn't the conversion missing in your suggestion?

as suggested in PR review
@valeros
Copy link
Member

valeros commented Feb 14, 2024

Isn't the conversion missing in your suggestion?

Maybe I'm missing something, what conversion do you mean? I expect the end user (or a board manifest) will set the image frequency in the correct format, e.g.:

platform = espressif32
board = ...
board_build.img_freq = 48m; instead of 48000000L

@Jason2866
Copy link
Contributor Author

Jason2866 commented Feb 14, 2024

This is inconsistent since for flash frequency and cpu frequency it is done like this:

    "f_cpu": "240000000L",
    "f_flash": "80000000L",

in boards.json Example for esp32-h2

@valeros
Copy link
Member

valeros commented Feb 14, 2024

I see, let's then refactor the code a bit, something like this should work:

def _normalize_frequency(frequency):
    frequency = str(frequency).replace("L", "")
    return str(int(int(frequency) / 1000000)) + "m"


def _get_board_f_flash(env):
    frequency = env.subst("$BOARD_F_FLASH")
    return _normalize_frequency(frequency)


def _get_board_img_freq(env):
    board_config = env.BoardConfig()
    if "build.img_freq" in board_config:
        return _normalize_frequency(board_config.get("build.img_freq"))

    return _get_board_f_flash(env)

Also, for the sake of consistency with f_cpu and f_flash, does it make sense to rename this new option to f_image?

@Jason2866
Copy link
Contributor Author

Perfect. f_image is better since it is more consist with the other options.
Changes done with last commit.

@Jason2866 Jason2866 changed the title Implement board_img_freq Implement get_board_f_image Feb 14, 2024
@Jason2866
Copy link
Contributor Author

@valeros Could you merge? Change is full backwards compatible

@valeros valeros merged commit 8fe5e12 into platformio:develop Feb 15, 2024
60 checks passed
@valeros
Copy link
Member

valeros commented Feb 15, 2024

Thanks for the PR, merged.

@Jason2866 Jason2866 deleted the patch-1 branch February 15, 2024 15:02
jonathandreyer pushed a commit to jonathandreyer/platform-espressif32 that referenced this pull request Nov 14, 2024
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