Add opt-in support for "LED_ENABLED" feature#15
Conversation
|
@osamuaoki, it'd be great if you could review this. Thanks! |
Makes LED support "opt-in" by defining "LED_ENABLED". Feature takes 6 bytes when enabled. Adds macros to handle active high and active low differences. Compared to Osamu's original implementation, moves turning on the LED to "run_bootloader", instead of "send_descriptor"; the behavior is pretty much the same, but it saves two bytes for the active-low case, since we only call `cbi` once when the bootloader becomes active (after setting T flag in SREG). Removes old and unused LED_PIN declaration.
volium
force-pushed
the
led_feature_support
branch
from
a8d97ce to
e3cf9e1
Compare
|
Good idea to make this OPT-IN. But I now see few issues. Before describing details, let me describe our situation.
Bootloader size can be 512B, 1024B, 2048B, 4096B. In light of this, I now think adding opt-in for 1024B bootloader by including sigprof's WIP branch to publish device ID etc. is good idea for which extra features are added while keeping size small enough to address windows issues. As for 0df6c69 ("Update comments, fix typos", 2022-01-20), I see issues for ProMicro for bootloader size which your code seems to address by moving "LED ON" earlier. But it comes with minor feature drop of LED behavior. I had bad contact problem with USB connector. So I wanted USB device not just powered but also properly recognized by the host. So, I intended for LED is:
I thought checking proper OS level USB device discovery with LED helped me to avoid monitoring dmsg on linux host. If that is not available, LED is only power-on verification. I didn't see much functional value. Of course, bright LED is nice to have when ON ... I recall my older PR #10 was including code size reduction idea from sigprof's repo so even promicro code fits into 512B. This is complicated and I skipped that part of code this time for cheery-picking. In e3cf9e1 ("Add opt-in support for "LED_ENABLED" feature", 2022-01-20), you left my comment in line56 as: I think this is not consistent with your modification. It's all 6 bytes for me by glancing your change. Any thought how you want to proceed? Osamu |
Moves "TURN_LED_ON" within "SET_CONFIGURATION" so LED is turned on ONLY when device has gone through enumeration. Adds check for "LED_ACTIVE_LEVEL" to turn LED off at the beginning of "run_bootloader" if LED is active low. This has a 2-byte penalty which makes nanoBoot go beyond 512 bytes on ProMicro-compatible boards.
|
Hey @osamuaoki, thank you for the feedback, I have addressed the issues you mentioned in the original commit. With this last round of changes, you are correct, the code no longer fits on ProMicro-compatible boards, though. I think we should try to include at least one more optimization to make it fit everywhere, but I think we can/should do that on a separate PR, for now, with I have validated the change; moving I think if/when @sigprof makes more progress to include device descriptors (which I also think should be an opt-in feature), we should definitely allow the code to grow beyond the 512-byte limit (1024B). Let me know what you think... thanks again! |
|
Hi #16 offer major size reduction. As for descriptor, it may fit in. I am not sure if removing "cli" is good idea, though. Maybe we can do descriptor without dropping cli. YH is always 0. So may be we can use it as rZERO (Is there any good use of r0 now?) Also, after real reset, registers should be zeroed. So XOR call to set registers to zero may be dropped. (The caveat is we may get software reset by a simple JMP, then registers will not be zerod.) rZERO, rONE, YH initialization should give us 6 bytes. If size is tight, we can trim ID name from "nanoBoot" to "nB" (6 more bytes saved) :-) Just a thought. I only have one unused ATmega32u4 board. ... Good night ;-) |
|
Hi #16 is done which covers fix to promicro. |
`set_watchdog_timer` was called in two places, but the parameter value passed in r16 (the value used to unlock the watchdog configuration) was the same in both cases. Move the duplicated initialization of r16 into the `set_watchdog_timer` function itself, saving 2 bytes.
|
Hey @osamuaoki, thanks for taking another look at this change. I looked at PR #16 and I cherry-picked @sigprof's c2b4ba5 into this PR, so now LED can be enabled across ALL hardware variants without issues (512 bytes when using I am going to merge this change now, so that we can work exclusively on the size optimizations and feature improvements separately. |
3 participants
Makes LED support "opt-in" by defining "LED_ENABLED". Feature takes 6 bytes when enabled.
Adds macros to handle active high and active low differences.
Compared to @osamuaoki's original implementation, moves turning on the LED to "run_bootloader", instead of "send_descriptor"; the behavior is pretty much the same, but it saves two bytes for the active-low case, since we only call
cbionce when the bootloader becomes active (after setting T flag in SREG).Removes old and unused LED_PIN declaration.