SpenceKonde / ATTinyCore

Arduino core for ATtiny 1634, 828, x313, x4, x41, x5, x61, x7 and x8
Other
1.53k stars 301 forks source link

Fix optiboot hex file paths #790

Closed ahshah closed 10 months ago

ahshah commented 11 months ago
SpenceKonde commented 11 months ago

Thanks - I've got one question, and one concern: attiny1634micr.menu.clock.internal_1m.build.extra_flags=-DBOOT_TUNED120

What exactly is this aimed at? I don't think that iwlll correctly get you 1 MHz

And, I see this line

!/usr/bin/python3

in the python file. Now I run windows. I don't think that line will work from windows will it? Won't it prevent the script from running?

ahshah commented 11 months ago

What exactly is this aimed at? I don't think that iwlll correctly get you 1 MHz

Great question- I have no idea. It looks like boards.txt and the script have been out of sync. My changes in the python generation file are not intended to make modifications to htis portion of the file. Since theres a discrepancy from whats been generated and whats already existed, I added it to the review as is as it seems like an oversight rather than a bug. This atleast brings the two back into sync. My opinion is that if this is something that breaks functionality, then we should remove the offending line from the script, but I am not on the 1634 platform so I have no way of testing which is correct.

in the python file. Now I run windows. I don't think that line will work from windows will it? Won't it prevent the script from running?

Not really, it should be benign, seeing as # is a comment keyword in python. There are good reasons for adding this.

It depends if cross platform compatibility for running internal tools is important and within the scope of this project. I work in BSD and *nix. If you think this is important (and on second thought) it should probably use: /usr/bin/env python3 For cross platform compatibility. Windows users can use pylauncher, https://bitbucket.org/vinay.sajip/pylauncher/downloads/

Another argument in favor of this is that future developers who are not on windows may also want to add the #! statement as it is pretty ubiquitous on this side of the OS wall.

SpenceKonde commented 10 months ago

I think I remembered what I was doing with the 1 MHz boot tuned 12 - you cant change what the boot tuning is without reloading the bootloader, and I think the logic I have is that if F_CPU requested is 1 MHz, but we start up at 12 from boot tuning for micronucleus, we reload the factory cal and enable the prescaler.

Thanks

ahshah commented 10 months ago

Thanks for merging! I noticed some general formatting checks failed- if there are improvements on how to ease such errors for the next pull request, please let me know.

SpenceKonde commented 10 months ago

They weren't passing before. What matters are the compile tests. Which we DONT HAVE RIGHT NOW so we CANT RELEASE ANYTHING! I have no fucking clue what parts do and don't compile blink under what conditions.

Anything that's in a variant or library (but not the core itself, because it's ifdef hells are unnavigable if we comply with Astyle's suggestions, which prohibit indentation for ifdefs) should pass astyles checks, or the astyle should be turned off (see the megatinycore repo, megaavr/libraries/tinyNeoPixel (very very similar to the one here) - look at the comments around the start of show() - something like / INDENT-OFF/ on it's own line or something, and the opposite to turn it back on after the part that it wants to make unreadable (readable code is more important than making astyle happy.

The general formating checks are to deal with frequent sins that get in the way of development in general - whitespace at start of files, whitespace at end of lines, and so on. I use sublime text, and it automatically fixes the "must have blank line at EoF" and "whitespace at end of line" things (they're options that I had to enable, but it's been a real blessing), though not the blank lines at start of file. Also the presence of tabs anywhere except keywords.txt is strictly prohibited, and I'd prohibit it there to except that then keywords.txt wouldn't work, because arduino requires tabs to be used as the separator.

(I actually use a different editor tool entirely for keywords.txt files - because I can turn automatically indent with spaces on and off based on file type, but not file name, so it's always enabled, and that SUCKS if you have to edit one file that has to use tabs not spaces, but any other file will fail the CI tests if you have a single tab anywhere in the file :-P And like, it's mighty easy to confuse a tab and a space...

ahshah commented 10 months ago

What is the intention for compile tests? To ensure that compilation is not broken? if so, is there a framework we can bring in to help with that? Or an example of how to write such a test? The IDE hides alot of compilation time details. So an example of how to go about these tests would be great.

I feel the project needs both compile tests, example hook ups, and example code. Ideally for every supported chip. Oh, and I want a pony too.

SpenceKonde commented 10 months ago

The CI tests are to make sure compilation works. See the DxCore actions for an example of what the intent there is. There's another issue open where this is being discussed.

As for example hookups and example code, well - I see that you know how to write a PR so.... (also there are library examples, so there is some example code - those are the files that get used by the compile tests over on DxC/mTC, in fact)

ahshah commented 10 months ago

Issue still exists on commit 747d8533b747758be3bc4e91eb5c35a05057467c It looks like I did not get this PR right- I am still having an issue when I resynced the codeline for the 841 serial fixes. Please stay tuned.