Re: [PR] Patches to support AVRDA/DB created by KR [nuttx]
xiaoxiang781216 merged PR #16323: URL: https://github.com/apache/nuttx/pull/16323 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Patches to support AVRDA/DB created by KR [nuttx]
acassis commented on PR #16323: URL: https://github.com/apache/nuttx/pull/16323#issuecomment-2860992196 Fixed! Please @xiaoxiang781216 @jerpelea @cederom @lupyuen review/approve again -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Patches to support AVRDA/DB created by KR [nuttx]
acassis commented on PR #16323: URL: https://github.com/apache/nuttx/pull/16323#issuecomment-2860966755 Aff, I did a mistake, I created a new commit and replaced the old commits. I will send them again -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Patches to support AVRDA/DB created by KR [nuttx]
cederom commented on PR #16323: URL: https://github.com/apache/nuttx/pull/16323#issuecomment-2860048911 @acassis yes its already updated in https://github.com/apache/nuttx/pull/16336 please approve the PR so it can be merged :-) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Patches to support AVRDA/DB created by KR [nuttx]
acassis commented on PR #16323: URL: https://github.com/apache/nuttx/pull/16323#issuecomment-2859650450 @cederom maybe it is better to remove this rule after the PR was submitted. So, be in sync with the master (upstream) is important when opening the PR, after that the github will do the rebase automatically. See: "This branch is out-of-date with the base branch", it means everybody will need to update all PR just because some PR was integrated in the mean time. It will require more (unnecessary) time from committer to updating the PRs -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Patches to support AVRDA/DB created by KR [nuttx]
xiaoxiang781216 commented on code in PR #16323: URL: https://github.com/apache/nuttx/pull/16323#discussion_r2078090071 ## arch/avr/src/avr/Kconfig: ## @@ -57,6 +57,26 @@ config AVR_BUILDROOT_TOOLCHAIN endchoice # Toolchain +config AVR_LINUXGCC_TOOLCHAIN_IS_GCC + bool "Mark the toolchain as a GCC toolchain" + default n + depends on AVR_LINUXGCC_TOOLCHAIN + select ARCH_TOOLCHAIN_GCC Review Comment: Done. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Patches to support AVRDA/DB created by KR [nuttx]
xiaoxiang781216 commented on code in PR #16323: URL: https://github.com/apache/nuttx/pull/16323#discussion_r2078090721 ## arch/avr/src/avr/Kconfig: ## @@ -57,6 +57,26 @@ config AVR_BUILDROOT_TOOLCHAIN endchoice # Toolchain +config AVR_LINUXGCC_TOOLCHAIN_IS_GCC + bool "Mark the toolchain as a GCC toolchain" + default n + depends on AVR_LINUXGCC_TOOLCHAIN + select ARCH_TOOLCHAIN_GCC Review Comment: we can merging this PR first, and fix the problem later if you want. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Patches to support AVRDA/DB created by KR [nuttx]
acassis commented on code in PR #16323: URL: https://github.com/apache/nuttx/pull/16323#discussion_r2077566675 ## arch/avr/src/avr/Kconfig: ## @@ -57,6 +57,26 @@ config AVR_BUILDROOT_TOOLCHAIN endchoice # Toolchain +config AVR_LINUXGCC_TOOLCHAIN_IS_GCC + bool "Mark the toolchain as a GCC toolchain" + default n + depends on AVR_LINUXGCC_TOOLCHAIN + select ARCH_TOOLCHAIN_GCC Review Comment: @xiaoxiang781216 could you please keep this discussion in the mailing list? Since KR is not here to respond, I suggest you align with him in the mailing list and then I do the suggested modifications. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Patches to support AVRDA/DB created by KR [nuttx]
xiaoxiang781216 commented on code in PR #16323: URL: https://github.com/apache/nuttx/pull/16323#discussion_r2076033609 ## arch/avr/src/avr/Kconfig: ## @@ -57,6 +57,26 @@ config AVR_BUILDROOT_TOOLCHAIN endchoice # Toolchain +config AVR_LINUXGCC_TOOLCHAIN_IS_GCC + bool "Mark the toolchain as a GCC toolchain" + default n + depends on AVR_LINUXGCC_TOOLCHAIN + select ARCH_TOOLCHAIN_GCC Review Comment: we can disable DEBUG_OPT_UNUSED_SECTIONS from defconfig or verify all boards add KEEP, it isn't good to keep the ugly code, or please drop this patch directly. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Patches to support AVRDA/DB created by KR [nuttx]
acassis commented on code in PR #16323: URL: https://github.com/apache/nuttx/pull/16323#discussion_r2076029293 ## arch/avr/src/avr/Kconfig: ## @@ -57,6 +57,26 @@ config AVR_BUILDROOT_TOOLCHAIN endchoice # Toolchain +config AVR_LINUXGCC_TOOLCHAIN_IS_GCC + bool "Mark the toolchain as a GCC toolchain" + default n + depends on AVR_LINUXGCC_TOOLCHAIN + select ARCH_TOOLCHAIN_GCC Review Comment: Hi @xiaoxiang781216 actually he explained why he decided to do this way in an email in the mailing list: ``` [ Enable eliminating unused sections with GCC ] More precisely, this patch allows marking the GCC toolchain as a GCC toolchain (ie. GCC toolchain option in AVR-specific Kconfig will select ARCH_TOOLCHAIN_GCC.) The direct consequence is that DEBUG_OPT_UNUSED_SECTIONS is enabled (it is marked default Y) and unused code is removed from the resulting binary. Other architectures have this enabled, program code savings can be quite relavant for these flash-constrained chips and as far as I understand it, the downsides are negligible. However, linker scripts need to account for this and, for existing chips, they do not. Most notably, .vectors sections need to be flagged as KEEP, otherwise the linker removes them too. Since I can only test the DA chips properly, I enabled this indirectly. Instead of selecting ARCH_TOOLCHAIN_GCC unconditionally, the GCC toolchain only enables additional option marked as default N. This additional option then selects ARCH_TOOLCHAIN_GCC, if set by user; its description provides warning about the linker script. ``` According to him: "doing it like that would likely break existing AVR boards, some code sections are not-referenced from the linker's point of view and therefore removed - this affects for example .vectors section." This goal it to fix it later for all boards, but now it is a fail-safe solution -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Patches to support AVRDA/DB created by KR [nuttx]
acassis commented on code in PR #16323: URL: https://github.com/apache/nuttx/pull/16323#discussion_r2076029293 ## arch/avr/src/avr/Kconfig: ## @@ -57,6 +57,26 @@ config AVR_BUILDROOT_TOOLCHAIN endchoice # Toolchain +config AVR_LINUXGCC_TOOLCHAIN_IS_GCC + bool "Mark the toolchain as a GCC toolchain" + default n + depends on AVR_LINUXGCC_TOOLCHAIN + select ARCH_TOOLCHAIN_GCC Review Comment: Hi @xiaoxiang781216 actually he explained why he decided to do this way: ``` [ Enable eliminating unused sections with GCC ] More precisely, this patch allows marking the GCC toolchain as a GCC toolchain (ie. GCC toolchain option in AVR-specific Kconfig will select ARCH_TOOLCHAIN_GCC.) The direct consequence is that DEBUG_OPT_UNUSED_SECTIONS is enabled (it is marked default Y) and unused code is removed from the resulting binary. Other architectures have this enabled, program code savings can be quite relavant for these flash-constrained chips and as far as I understand it, the downsides are negligible. However, linker scripts need to account for this and, for existing chips, they do not. Most notably, .vectors sections need to be flagged as KEEP, otherwise the linker removes them too. Since I can only test the DA chips properly, I enabled this indirectly. Instead of selecting ARCH_TOOLCHAIN_GCC unconditionally, the GCC toolchain only enables additional option marked as default N. This additional option then selects ARCH_TOOLCHAIN_GCC, if set by user; its description provides warning about the linker script. ``` According to him: "doing it like that would likely break existing AVR boards, some code sections are not-referenced from the linker's point of view and therefore removed - this affects for example .vectors section." This goal it to fix it later for all boards, but now it is a fail-safe solution -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Patches to support AVRDA/DB created by KR [nuttx]
xiaoxiang781216 commented on code in PR #16323: URL: https://github.com/apache/nuttx/pull/16323#discussion_r2075996375 ## arch/avr/src/avr/Kconfig: ## @@ -57,6 +57,26 @@ config AVR_BUILDROOT_TOOLCHAIN endchoice # Toolchain +config AVR_LINUXGCC_TOOLCHAIN_IS_GCC + bool "Mark the toolchain as a GCC toolchain" + default n + depends on AVR_LINUXGCC_TOOLCHAIN + select ARCH_TOOLCHAIN_GCC Review Comment: but I don't see the change at all -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Patches to support AVRDA/DB created by KR [nuttx]
acassis commented on PR #16323: URL: https://github.com/apache/nuttx/pull/16323#issuecomment-2855429661 > Thank you @acassis :-) > > These patches come from outside private repository, and processed by @acassis, I have asked original author to follow Contributing Guidelines and sign the commits :-) > > Can you provide short build instructions and what targets are used? I would like to see if that builds on FreeBSD and maybe I have a board to run :-) Hi @cederom I just followed the suggestions that he added in the email -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Patches to support AVRDA/DB created by KR [nuttx]
acassis commented on PR #16323: URL: https://github.com/apache/nuttx/pull/16323#issuecomment-2855087468 > > @xiaoxiang781216 @lupyuen should we disable AVRDA/DB check for now until we get a proper AVR Toolchain? > > hi @acassis you must add this > > ``` > # Sparc-gaisler-elf toolchain doesn't provide macOS binaries > /sparc > -xx3823:nsh > > -breadxavr:hello > ``` > > in tools/ci/testlist/other.dat > > https://github.com/apache/nuttx/blob/e086ef2d029fbceb2a3d9bea070f3d286535aa29/tools/ci/testlist/other.dat#L16 Thank you @simbit18 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Patches to support AVRDA/DB created by KR [nuttx]
simbit18 commented on PR #16323: URL: https://github.com/apache/nuttx/pull/16323#issuecomment-2854893902 > @xiaoxiang781216 @lupyuen should we disable AVRDA/DB check for now until we get a proper AVR Toolchain? hi @acassis you must add this ``` # Sparc-gaisler-elf toolchain doesn't provide macOS binaries /sparc -xx3823:nsh -breadxavr:hello ``` in tools/ci/testlist/other.dat https://github.com/apache/nuttx/blob/e086ef2d029fbceb2a3d9bea070f3d286535aa29/tools/ci/testlist/other.dat#L16 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Patches to support AVRDA/DB created by KR [nuttx]
acassis commented on PR #16323: URL: https://github.com/apache/nuttx/pull/16323#issuecomment-2854607607 @xiaoxiang781216 @lupyuen should we disable AVRDA/DB check for now until we get a proper AVR Toolchain? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Patches to support AVRDA/DB created by KR [nuttx]
acassis commented on code in PR #16323: URL: https://github.com/apache/nuttx/pull/16323#discussion_r2075394221 ## arch/avr/src/avr/Kconfig: ## @@ -57,6 +57,26 @@ config AVR_BUILDROOT_TOOLCHAIN endchoice # Toolchain +config AVR_LINUXGCC_TOOLCHAIN_IS_GCC + bool "Mark the toolchain as a GCC toolchain" + default n + depends on AVR_LINUXGCC_TOOLCHAIN + select ARCH_TOOLCHAIN_GCC Review Comment: Good point! I noticed that CONFIG_AVR_LINUXGCC_TOOLCHAIN_IS_GCC is not used in the code -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Patches to support AVRDA/DB created by KR [nuttx]
xiaoxiang781216 commented on code in PR #16323: URL: https://github.com/apache/nuttx/pull/16323#discussion_r2074659948 ## arch/avr/src/avr/Kconfig: ## @@ -57,6 +57,26 @@ config AVR_BUILDROOT_TOOLCHAIN endchoice # Toolchain +config AVR_LINUXGCC_TOOLCHAIN_IS_GCC + bool "Mark the toolchain as a GCC toolchain" + default n + depends on AVR_LINUXGCC_TOOLCHAIN + select ARCH_TOOLCHAIN_GCC Review Comment: why not select ARCH_TOOLCHAIN_GCC in AVR_LINUXGCC_TOOLCHAIN directly -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Patches to support AVRDA/DB created by KR [nuttx]
xiaoxiang781216 commented on PR #16323: URL: https://github.com/apache/nuttx/pull/16323#issuecomment-2853129084 > @sumpfralle please add RCALL to codespell exception, it is failing: > > /home/runner/work/nuttx/nuttx/nuttx/arch/avr/src/avr/excptmacros.h:336: RCALL ==> RECALL > > RCALL is a valid AVR instruction but why not add in this PR directly? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Patches to support AVRDA/DB created by KR [nuttx]
acassis commented on PR #16323: URL: https://github.com/apache/nuttx/pull/16323#issuecomment-2852893321 @sumpfralle please add RCALL to codespell exception -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Patches to support AVRDA/DB created by KR [nuttx]
nuttxpr commented on PR #16323: URL: https://github.com/apache/nuttx/pull/16323#issuecomment-2852887823 [**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR does not fully meet the NuttX requirements. While it provides a summary and mentions impact, it lacks crucial details. Here's a breakdown of missing information: * **Summary:** Lacks specifics. *What* functional part of the code changed? *How* does enabling AVRDA/DB support work (e.g., new drivers, board configuration files, modifications to existing code)? Are there related NuttX issues? * **Impact:** While user impact is mentioned, it needs to address *all* other impact categories (build process, hardware, documentation, security, compatibility). Simply stating "NO" is insufficient; justify why there's no impact or provide details if there is. * **Testing:** "Breadboard" is not adequate testing information. The requirements ask for specific host and target details (OS, CPU, compiler, architecture, board, configuration) and, crucially, *testing logs*. Without logs demonstrating the functionality before and after the change, there's no verifiable proof that the changes work as intended. Therefore, the PR needs substantial revision before it can be considered complete. The author needs to provide the missing details to ensure the changes are properly documented, reviewed, and integrated. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
