Skip to content

Conversation

@vladopajic
Copy link

having 2024 as default year will make it less annoying to configure year

@github-actions
Copy link

Build checks have not completed. Possible reasons for this are:

  1. The checks need to be approved by a maintainer
  2. The branch has conflicts
  3. The firmware build has failed

@NeroBurner NeroBurner added the enhancement Enhancement to an existing app/feature label Sep 18, 2024
Copy link
Contributor

@NeroBurner NeroBurner left a comment

Choose a reason for hiding this comment

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

We probably have to update the variable year by year then, but I think it makes sense to have the default year close the the release date of the firmware

@NeroBurner NeroBurner requested a review from a team September 18, 2024 19:54
@NeroBurner NeroBurner added this to the 1.15.0 milestone Sep 28, 2024
@mark9064
Copy link
Member

mark9064 commented Sep 28, 2024

One option would be using the compiler to insert the current year at time of compilation, I'm sure that's possible as the build date page in settings already exists

Edit: looks like __DATE__ + 7 works in theory? Might generate some warnings though
Edit edit: atoi(__DATE__ + 7) compiles with no warnings on GCC 13 at least, but I haven't tested if it actually works correctly

Copy link
Member

@mark9064 mark9064 left a comment

Choose a reason for hiding this comment

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

This is the wrong place to introduce a default year. By changing the counter parameter instead the minimum year the spinbox can show is adjusted. The spinboxes otherwise display the current time. A better and more correct way to do this is to initialise the default time on boot to 1 Jan on the year of the compile time. This means that the time can still be adjusted freely. I would suspect that main.cpp where the time is loaded from the persistent RAM is the correct place to set the time as a fallback.

The compile time could be used as the default date and time, but I think this is confusing whereas setting to 1 Jan YEAR 00:00 is unambiguously a default time.

Copy link
Contributor

@NeroBurner NeroBurner left a comment

Choose a reason for hiding this comment

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

@mark9064 makes a very good point. Although changing the year to a time before now is esoteric I think, but taking away this option isn't nice as well.

@FintasticMan
Copy link
Member

I like the idea of using the __DATE__ macro to use the current year, but something bugs me about using atoi to do the conversion as that'll run at runtime rather than compile-time. That's why it could be fun (although completely unnecessary) to write a simple constexpr atoi-like function so that all of it can be done at compile time.

@mark9064
Copy link
Member

This attempt at constexpr atoi looks pretty bleak https://stackoverflow.com/a/59537554 😅
Maybe we only need part of it though

@FintasticMan
Copy link
Member

FintasticMan commented Sep 29, 2024

constexpr int isdigit_constexpr(int c) {
    return c >= '0' && c <= '9';
}

constexpr int atoi_constexpr(char const *str) {
    int result = 0;
    while (isdigit_constexpr(*str)) {
        result = result * 10 + *str - '0';
        str++;
    }
    return result;
}

I have no idea why that implementation looks so difficult, this is all it takes. I guess it could be that this isn't standards-compliant, and in that post they were trying to be as compliant as possible.

@mark9064
Copy link
Member

That looks much more reasonable and does the job here 👍

@vladopajic
Copy link
Author

@mark9064 thanks, good suggestions.

however i would not be able to make all those changes. my reasoning was to to make this PR because it is vary easy to change constant, instead of opening an issue about this.

please feel free to take over this PR or close it and make another one.

@mark9064
Copy link
Member

With #2133 this is integrated now and can be closed

@vladopajic vladopajic closed this Oct 28, 2024
@FintasticMan
Copy link
Member

@vladopajic Thank you for the idea of setting the year by default! And for the initial implementation, it was very helpful for working out the final solution.

@vladopajic
Copy link
Author

@vladopajic Thank you for the idea of setting the year by default! And for the initial implementation, it was very helpful for working out the final solution.

🙏

kudos to you guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Enhancement to an existing app/feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants