Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix flashinit panic not printing #8762

Merged
merged 11 commits into from
Dec 22, 2022
Merged

Conversation

mhightower83
Copy link
Contributor

@mhightower83 mhightower83 commented Dec 16, 2022

@mhightower83 mhightower83 changed the title fix panic not printing fix flashinit panic not printing Dec 16, 2022
cores/esp8266/flash_hal.h Outdated Show resolved Hide resolved
@d-a-v d-a-v added this to the 3.1 milestone Dec 16, 2022
@mcspr
Copy link
Collaborator

mcspr commented Dec 19, 2022

Just as a thought, don't we want to return something like this?

FlashInitResult flashinit();
struct FlashInitResult {
    bool ok { false };
    const char* err_msg;
};

@d-a-v
Copy link
Collaborator

d-a-v commented Dec 19, 2022

flashinit() was not designed to fail, so it is 'panic'ing instead

edit: my bad, I was some merged PRs late

@mhightower83
Copy link
Contributor Author

mhightower83 commented Dec 19, 2022

Too many merge conflicts were corrected with web edit, I need to correct some stuff and do a few real tests.

@mcspr
Copy link
Collaborator

mcspr commented Dec 19, 2022

flashinit() was not designed to fail, so it is 'panic'ing instead

I just mean the return value. Struct member name tells us what we mean right there, no need to remember triple meaning of a char pointer

@mhightower83
Copy link
Contributor Author

FlashInitResult flashinit();

FWIF: I tried this method and it increases "IROM code in flash" by 16 bytes.

@mcspr mcspr merged commit 137d421 into esp8266:master Dec 22, 2022
@mcspr
Copy link
Collaborator

mcspr commented Dec 22, 2022

FlashInitResult flashinit();

FWIF: I tried this method and it increases "IROM code in flash" by 16 bytes.

Suppose, bool could go, but methods could do the thing instead

#include <cstdio>

void __panic_func(const char* file, int line, const char* func) {
    printf("panic! at %s:%d::%s\n", file, line, func);
}

struct SimpleResult {
    SimpleResult() = default;
    constexpr SimpleResult(const char* error) :
        _error(error)
    {}

    bool ok() const {
        return _error == nullptr;
    }

    const char* error() const {
        return _error;
    }

    // or, other order, depends on whats more important to replace
    void check(const char* file = __builtin_FILE(),
            int line = __builtin_LINE(),
            const char* func = __builtin_FUNCTION())
    {
        if (!ok()) {
            __panic_func(file, line, func);
        }
    }

private:
    const char* _error { nullptr };
};

int main() {
    SimpleResult what{"oops"};
    what.check();
}

@mhightower83 mhightower83 deleted the pr-autoconfig-fix branch December 22, 2022 18:07
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants