You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/12/18 19:45:18 UTC
[GitHub] [incubator-nuttx-apps] nandojve opened a new pull request #929: MCUboot move apps to examples
nandojve opened a new pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929
## Summary
The current version of MCUboot have core and examples together. This moves samples to apps/examples directory to clean up. It add another example that is useful to test switch between two valid images using nsh.
## Impact
Require update board configs at NuttX
## Testing
samv7-xult
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] nandojve commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
nandojve commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1054243732
Hi @acassis,
This now fixes all comments from @gustavonihei to move to examples.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1057161737
> I still need a day to think about this change. In general I support a motivation of moving things to `apps/examples` and group related functionality in `mcuboot` folder. I'm writing this because when I'm looking into `lvgldemo` example that seems natural to me and I think that possibly examples for other libraries (third party code) should follow the similar way. I will examine other use cases as well and get back to this change with the update.
`lvgldemo` is a recipe for building the former `lv_demos` project, which was a dedicated repository just for the LVGL demo apps. I understand that as a justification to live in the `examples` folder.
But, after LVGL 8.2 release `lv_demos` has been integrated to the `lvgl` project, making the other repository obsolete:
https://github.com/lvgl/lv_demos
https://github.com/lvgl/lvgl/tree/master/demos
Once LVGL gets updated to the latest release, I believe `examples/lvgldemo` should be deleted and the applicable options should appear alongside `graphics/lvgl`.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei edited a comment on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1056941297
> Hi Folks,
>
> This change of feelings is that create bad projects. I invested a lot of time to put things in the correct way and, after weeks, now, it is about feelings. Sorry, but this is very disrespectful.
>
I understand that you invested time in this PR and that you may feel it is being undervalued.
This PR has been open for months, but reviewed just by a single person (me).
My feelings about moving the examples to the `examples` folder should not be new to you @nandojve, as we've discussed several times before, so on my side there is no "change of feelings". If I really agreed to this, I would have already done this way from the very beginning. I still feel that the examples from a given library should be placed in the same level.
Even so, I haven't complained about this in this PR. My expectation was to see if the other developers were OK with it, without adding any bias.
The points I raised on this PR is to prevent further deviations from the project structure, such as the naming of the Kconfig options. So, in case anyone approved it, at least it is being doing the most correct way.
> If people from NuttX detected a issue about where examples should be added, that should be documented (no more feelings), that it. Since the issue was detected in this PR, and PR was complete reworked to be as requested, at least, you folks should respect the work done and approve it.
I am responsible for the code I approve to be merged to the project's codebase. And so far I am still not confident in doing so, according to the reason I stated on the previous message.
Let's wait on the feedback from others.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] nandojve commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
nandojve commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r772601755
##########
File path: examples/mcuboot/ota/Kconfig
##########
@@ -0,0 +1,56 @@
+#
+# For a description of the syntax of this configuration file,
+# see the file kconfig-language.txt in the NuttX tools repository.
+#
+
+menuconfig EXAMPLES_MCUBOOT_OTA
Review comment:
I'll rework PR as a sequence, changes are not superfluous. The simple fact that this refactor is necessary shows that MCUboot, as is, have inconsistencies. This helps to improve into a more generic and easy to use way. Since it is required replace strings in the main NuttX repo, it is better perform once with right value, instead twice.
Maybe `EXAMPLES_MCUBOOT_UPDATE` is a more consistent prefix name and `_AGENT/_CONFIRM` better suffixes.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r773105477
##########
File path: examples/mcuboot/update_agent/Makefile
##########
@@ -0,0 +1,41 @@
+############################################################################
+# apps/examples/mcuboot/update_agent/Makefile
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership. The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+include $(APPDIR)/Make.defs
+
+ifneq ($(CONFIG_EXAMPLES_MCUBOOT_UPDATE_AGENT),)
+MAINSRC += mcuboot_agent_main.c
+
+PROGNAME += mcuboot_agent
+PRIORITY += SCHED_PRIORITY_DEFAULT
+STACKSIZE += $(CONFIG_DEFAULT_TASK_STACKSIZE)
+endif
+
+ifneq ($(CONFIG_EXAMPLES_MCUBOOT_UPDATE_AGENT_SLOT_CONFIRM),)
+MAINSRC += mcuboot_confirm_main.c
+
+PROGNAME += mcuboot_confirm
+PRIORITY += SCHED_PRIORITY_DEFAULT
+STACKSIZE += $(CONFIG_DEFAULT_TASK_STACKSIZE)
+endif
Review comment:
The Update Agent and the Confirm are two separate examples.
According to the new structure, they should be kept in separate directories as well.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r773105477
##########
File path: examples/mcuboot/update_agent/Makefile
##########
@@ -0,0 +1,41 @@
+############################################################################
+# apps/examples/mcuboot/update_agent/Makefile
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership. The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+include $(APPDIR)/Make.defs
+
+ifneq ($(CONFIG_EXAMPLES_MCUBOOT_UPDATE_AGENT),)
+MAINSRC += mcuboot_agent_main.c
+
+PROGNAME += mcuboot_agent
+PRIORITY += SCHED_PRIORITY_DEFAULT
+STACKSIZE += $(CONFIG_DEFAULT_TASK_STACKSIZE)
+endif
+
+ifneq ($(CONFIG_EXAMPLES_MCUBOOT_UPDATE_AGENT_SLOT_CONFIRM),)
+MAINSRC += mcuboot_confirm_main.c
+
+PROGNAME += mcuboot_confirm
+PRIORITY += SCHED_PRIORITY_DEFAULT
+STACKSIZE += $(CONFIG_DEFAULT_TASK_STACKSIZE)
+endif
Review comment:
The Update Agent and the Confirm are two separate examples.
They should not be considered as a single `update_agent` example inside `examples/mcuboot`.
I'd suggest one of these two approaches:
- Simply move the examples to the `examples/mcuboot`, without the `update_agent` folder (**preferred**)
- Separate them, each one into its own folder (e.g. `examples/mcuboot/update_agent` and `examples/mcuboot/confirm`).
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r773126229
##########
File path: examples/mcuboot/update_agent/Kconfig
##########
@@ -0,0 +1,44 @@
+#
+# For a description of the syntax of this configuration file,
+# see the file kconfig-language.txt in the NuttX tools repository.
+#
+
+config EXAMPLES_MCUBOOT_UPDATE_AGENT
+ bool "MCUboot update agent example"
+ default n
+ depends on NET_TCP
+ ---help---
+ Example application that implements an update agent that downloads
+ an application firmware image from a given URL and saves it to the
+ secondary slot as a pending update.
+
+if EXAMPLES_MCUBOOT_UPDATE_AGENT
+
+config EXAMPLES_MCUBOOT_UPDATE_AGENT_UPDATE_URL
+ string "URL for update image"
+ default ""
+
+config EXAMPLES_MCUBOOT_UPDATE_AGENT_DL_BUFFER_SIZE
+ int "Download buffer size in bytes"
+ default 512
+
+config EXAMPLES_MCUBOOT_UPDATE_AGENT_DL_VERIFY_MD5
+ bool "Calculate MD5 of update image"
+ default n
+ depends on CODECS_HASH_MD5
+
+config EXAMPLES_MCUBOOT_UPDATE_AGENT_DL_MD5_HASH
+ string "Expected MD5 sum of update image"
+ default ""
+ depends on EXAMPLES_MCUBOOT_UPDATE_AGENT_DL_VERIFY_MD5
+
+config EXAMPLES_MCUBOOT_UPDATE_AGENT_SLOT_CONFIRM
+ tristate "MCUboot slot confirm example"
+ default n
+ ---help---
+ Example application for confirming a newly installed application
+ application firmware image using MCUboot public APIs.
+ This application should be used as the OTA update package of the
+ EXAMPLES_MCUBOOT_UPDATE_AGENT example.
Review comment:
This should be moved as well to a dedicated folder for the `confirm` example.
The `Update Agent` and the `Confirm` example have no coupling between them.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] nandojve commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
nandojve commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1045269835
> > > @nandojve how about this patch?
> >
> >
> > Indeed, this is taking more time than expected. I think I can recap it in one maybe two weeks.
> > BTW, I noted that there is not available option to move to draft. That could be helpful in this kind of situation because I could just move to draft and when it is ready again move to open.
>
> There is an option "move to draft" it is just hidden well by GitHub UI :)
Uau, I need to magnify 1M x to find it!
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] nandojve commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
nandojve commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1056917598
Hi Folks,
This change of feelings is that create bad projects. I invested a lot of time to put things in the correct way and, after weeks, now, it is about feelings. Sorry, but this is very disrespectful.
If people from NuttX detected a issue about where examples should be added, that should be documented (no more feelings), that it. Since the issue was detected in this PR, and PR was complete reworked to be as requested, at least, you folks should respect the work done and approve it.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] acassis commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1057246858
@nandojve and @otavio I understand your concerns, but in fact we don't have a guideline for this kind of definition, because normally when someone decide to contribute a code to NuttX they follow the pre-existent rule/definition. In this case (bootloader, mcuboot), we already had an apps/boot/ to it. In the past NuttX had basically only the examples/ directory, but because it was getting more are more applications inside it, we decided to categorize it, as you can see on menuconfig "Application Configuration". BTW, we don't have "Code Owners" on NuttX, we follow the Apache Way (https://www.apache.org/theapacheway/) and everybody here is voluntary, so please don't get offended when someone delay to review your 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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] otavio commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
otavio commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1056984566
@gustavonihei I think @nandojve is a valid concern. However, this cannot be based on personal feelings but on a general project guideline that must be documented somewhere to avoid this.
A project as mature and big as NuttX should have this somewhere, so where is the guideline?
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] pkarashchenko commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1059271340
> > I still need a day to think about this change. In general I support a motivation of moving things to `apps/examples` and group related functionality in `mcuboot` folder. I'm writing this because when I'm looking into `lvgldemo` example that seems natural to me and I think that possibly examples for other libraries (third party code) should follow the similar way. I will examine other use cases as well and get back to this change with the update.
>
> `lvgldemo` is a recipe for building the former `lv_demos` project, which was a dedicated repository just for the LVGL demo apps. I understand that as a justification to live in the `examples` folder. But, after LVGL 8.2 release `lv_demos` has been integrated to the `lvgl` project, making the other repository obsolete: https://github.com/lvgl/lv_demos https://github.com/lvgl/lvgl/tree/master/demos
>
> Once LVGL gets updated to the latest release in `nuttx-apps`, I believe `examples/lvgldemo` should be deleted and the applicable options should appear alongside `graphics/lvgl`.
Yes. I understand that. But there is a difference with MCUboot case. The MCUboot examples that are a part of this PR are not provided by MCUboot and are an example on how to use third party code like `examples/mqttc` is an example on top of `NETUTILS_MQTTC`. If those examples reside inside the MCUboot repository than it would be logically to keep integration file inside the `boot/mcuboot` folder.
I think that we should go in the next way:
1. Move MCUboot examples to `examples` folder.
2. Rename `boot` to `bootutils`.
I also just noticed that there is some inconsistency with `modbus` that is located not inside the `netutils`. That is also something for me to think about.
So to summarize: I'm fine to continue with this PR.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1057018989
I am fine with either place. I personally follow this order:
1. If this program is general enough to be used in many case without the code modification, it's better to move out of apps/examples folder
2. Otherwise, it's better to put into the corresponding subsystem folder to indicate that it's very mature and ready for use immediately.
Again, many application doesn't follow this, so I am fine with the change.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r772388578
##########
File path: examples/mcuboot_ota/Kconfig
##########
@@ -0,0 +1,63 @@
+#
+# For a description of the syntax of this configuration file,
+# see the file kconfig-language.txt in the NuttX tools repository.
+#
+
+menuconfig EXAMPLES_MCUBOOT_OTA
+ tristate "MCUboot OTA example"
+ default n
+ select BOOT_MCUBOOT
+ ---help---
+ Example application demonstrate a software firmware update.
+ It downloads an application firmware image from a given URL.
+
+if EXAMPLES_MCUBOOT_OTA
+
+comment "This example requires network"
+ depends on !NET_TCP
+
+choice EXAMPLES_MCUBOOT_OTA_APP
+ prompt "Select MCUboot application"
+ default EXAMPLES_MCUBOOT_OTA_APP_AGENT
+ depends on NET_TCP
+ ---help---
+ Select MCUboot application.
+
+config EXAMPLES_MCUBOOT_OTA_APP_AGENT
+ bool "Agent"
+ ---help---
+ Implements the update agent that downloads the application
+ firmware image from a given URL and saves it to the secondary
+ slot as a pending update.
+
+config EXAMPLES_MCUBOOT_OTA_APP_CONFIRM
+ bool "Confirm"
+ ---help---
+ Is the application to be download to demonstrate firmware
+ upgrade functionality.
+
+endchoice
Review comment:
I believe this is an unnecessary restriction.
One may create an application firmware image that includes both examples and create a testing loop performing update-confirm-repeat using the same base image.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1057002869
> @gustavonihei I think @nandojve is a valid concern. However, this cannot be based on personal feelings but on a general project guideline that must be documented somewhere to avoid this.
Sure, I completely agree with this.
I am sorry if I was not completely clear in my previous message, but I never prioritize personal preferences on my review activities. And if for some reason I need to add personal preferences to the review, I will proactively add this disclaimer.
> A project as mature and big as NuttX should have this somewhere, so where is the guideline?
As far as I know, there is no guideline. At least I am not aware of it.
And, as I stated in my previous comment, in the absence of documented guidelines, it all narrows down to personal preference.
Anyone is free and much welcome to contribute to the project documentation and improve it.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] pkarashchenko commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1073017039
Merging this with CI failure as there is dependency on PR in OS repository.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r772586627
##########
File path: examples/mcuboot/ota/Kconfig
##########
@@ -0,0 +1,56 @@
+#
+# For a description of the syntax of this configuration file,
+# see the file kconfig-language.txt in the NuttX tools repository.
+#
+
+menuconfig EXAMPLES_MCUBOOT_OTA
Review comment:
I don't think that OTA should be part of the name, since the update agent example does not restrict or assumes that the update will take place over a wireless network. And OTA does not imply "update".
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r772550909
##########
File path: examples/mcuboot/ota/Kconfig
##########
@@ -0,0 +1,56 @@
+#
+# For a description of the syntax of this configuration file,
+# see the file kconfig-language.txt in the NuttX tools repository.
+#
+
+menuconfig EXAMPLES_MCUBOOT_OTA
Review comment:
Could you explain the motivation for changing the application name?
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] nandojve commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
nandojve commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r772637956
##########
File path: examples/mcuboot/ota/Kconfig
##########
@@ -0,0 +1,56 @@
+#
+# For a description of the syntax of this configuration file,
+# see the file kconfig-language.txt in the NuttX tools repository.
+#
+
+menuconfig EXAMPLES_MCUBOOT_OTA
Review comment:
> If this is indeed necessary, the motivation for this refactor should be stated on the PR description. So far, I still consider these changes superfluous. The changes to the names and help text of Kconfig options, in my opinion, bring no improvement over the previous version.
_The current version of MCUboot have core and [define] example[s] together. This moves samples to apps/examples directory to clean up._ This is a clear motivation. The Kconfig name changes are necessary to keep name convention consistent once in all examples the Kconfig variables starts with EXAMPLES_.
> How does these changes in this PR cope with it?
The cleaning up require Kconfig name changes to keep consistency. This should be reflected into boards/configs/defconfig, which are related to MCUboot. README files may be affected because can describe how build/flash for users, for instance.
> The prefix is just EXAMPLES_MCUBOOT_. I don't understand how this notion of suffix fits here. There is no suffix in the Kconfig names.
The prefix is just the constant part of Kconfig symbol, which repeat in the whole Kconfig file example, nothing more. The suffix is the remaining part.
This PR moves MCUboot _samples_ to _a new application example_ inside examples/mcuboot/ota. For this initial proposal, prefix should be EXAMPLES_MCUBOOT_OTA. If this name is not OK I would like to change it. What do you suggest?
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r772647930
##########
File path: examples/mcuboot/ota/Kconfig
##########
@@ -0,0 +1,56 @@
+#
+# For a description of the syntax of this configuration file,
+# see the file kconfig-language.txt in the NuttX tools repository.
+#
+
+menuconfig EXAMPLES_MCUBOOT_OTA
Review comment:
We're not talking about the same issues here.
Regarding the code move, I am already okay with that, as I stated in my last comment. Also with the addition of `EXAMPLES_` to the Kconfig config names, which is related to the code move.
So, the previous `MCUBOOT_UPDATE_AGENT_EXAMPLE` key, once moved, should simply become `EXAMPLES_MCUBOOT_UPDATE_AGENT`. And that's it. That achieves the purpose of this PR.
That's why I said to restrict the PR to these changes only, to avoid hindering the merge.
I am not okay with everything else in this PR, because those other changes lack background.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-998750713
@nandojve Please, don't forget to run the `refresh.sh` tool for synchronizing the existing defconfigs to the latest changes to Kconfig entries.
`./tools/refresh.sh --silent <target>`
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1044742518
@nandojve how about this patch?
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1056897731
> @nandojve why to move mcuboot to examples? I think boot/ is a better place to keep it, what do you think @gustavonihei ? Actually there are many "examples" inside examples/ that we need to move to other place already.
I have mixed feelings about the placement of the examples. I understand @nandojve's intent, which kinda makes sense because already exists a dedicated **examples** folder holding example apps from some libs. But there are several others not following this same pattern.
So in the end this all narrows down to personal preference, until there is a consensus to decide the correct approach and someone submit a PR to standardize this.
**My** preference is to simply let the examples live alongside the respective library, makes it simpler for the users to find them and use them.
I will drop my previous review, because the requested changes were all applied as suggested.
But, as I stated above, I won't approve this change because I don't see a consistent reason to justify it.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r772609676
##########
File path: examples/mcuboot/ota/Kconfig
##########
@@ -0,0 +1,56 @@
+#
+# For a description of the syntax of this configuration file,
+# see the file kconfig-language.txt in the NuttX tools repository.
+#
+
+menuconfig EXAMPLES_MCUBOOT_OTA
Review comment:
> The simple fact that this refactor is necessary
If this is indeed necessary, the motivation for this refactor should be stated on the PR description. So far, I still consider these changes superfluous. The changes to the names and help text of Kconfig options, in my opinion, bring no improvement over the previous version.
> This helps to improve into a more generic and easy to use way. Since it is required replace strings in the main NuttX repo, it is better perform once with right value, instead twice.
How does these changes in this PR cope with it?
> Maybe EXAMPLES_MCUBOOT_UPDATE is a more consistent prefix name and _AGENT/_CONFIRM better suffixes.
The prefix is just `EXAMPLES_MCUBOOT_`. I don't understand how this notion of suffix fits here. There is no suffix in the Kconfig names.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 edited a comment on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1063617630
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] nandojve commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
nandojve commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1072438065
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei edited a comment on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1057161737
> I still need a day to think about this change. In general I support a motivation of moving things to `apps/examples` and group related functionality in `mcuboot` folder. I'm writing this because when I'm looking into `lvgldemo` example that seems natural to me and I think that possibly examples for other libraries (third party code) should follow the similar way. I will examine other use cases as well and get back to this change with the update.
`lvgldemo` is a recipe for building the former `lv_demos` project, which was a dedicated repository just for the LVGL demo apps. I understand that as a justification to live in the `examples` folder.
But, after LVGL 8.2 release `lv_demos` has been integrated to the `lvgl` project, making the other repository obsolete:
https://github.com/lvgl/lv_demos
https://github.com/lvgl/lvgl/tree/master/demos
Once LVGL gets updated to the latest release in `nuttx-apps`, I believe `examples/lvgldemo` should be deleted and the applicable options should appear alongside `graphics/lvgl`.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] acassis commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1054238587
@nandojve why to move mcuboot to examples? I think boot/ is a better place to keep it, what do you think @gustavonihei ? Actually there are many "examples" inside examples/ that we need to move to other place already.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] pkarashchenko commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1057111026
I still need a day to think about this change. In general I support a motivation of moving things to `apps/examples` and group related functionality in `mcuboot` folder. I'm writing this because when I'm looking into `lvgldemo` example that seems natural to me and I think that possibly examples for other libraries (third party code) should follow the similar way. I will examine other use cases as well and get back to this change with the update.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] nandojve commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
nandojve commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-998011355
> 1. Moving the existing examples to a new `examples/mcuboot` folder.
Done!
> 2. Adding the newly created example.
#934
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r772657189
##########
File path: examples/mcuboot/ota/Kconfig
##########
@@ -0,0 +1,56 @@
+#
+# For a description of the syntax of this configuration file,
+# see the file kconfig-language.txt in the NuttX tools repository.
+#
+
+menuconfig EXAMPLES_MCUBOOT_OTA
Review comment:
First: Why Update Agent is not a good name for the app?
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] nandojve commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
nandojve commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1053546033
depends on https://github.com/apache/incubator-nuttx/pull/5033
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r773105477
##########
File path: examples/mcuboot/update_agent/Makefile
##########
@@ -0,0 +1,41 @@
+############################################################################
+# apps/examples/mcuboot/update_agent/Makefile
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership. The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+include $(APPDIR)/Make.defs
+
+ifneq ($(CONFIG_EXAMPLES_MCUBOOT_UPDATE_AGENT),)
+MAINSRC += mcuboot_agent_main.c
+
+PROGNAME += mcuboot_agent
+PRIORITY += SCHED_PRIORITY_DEFAULT
+STACKSIZE += $(CONFIG_DEFAULT_TASK_STACKSIZE)
+endif
+
+ifneq ($(CONFIG_EXAMPLES_MCUBOOT_UPDATE_AGENT_SLOT_CONFIRM),)
+MAINSRC += mcuboot_confirm_main.c
+
+PROGNAME += mcuboot_confirm
+PRIORITY += SCHED_PRIORITY_DEFAULT
+STACKSIZE += $(CONFIG_DEFAULT_TASK_STACKSIZE)
+endif
Review comment:
The Update Agent and the Confirm are two separate examples.
Since this PR intends to move them to the `examples` folder, they should be kept in separate directories as well.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r773105477
##########
File path: examples/mcuboot/update_agent/Makefile
##########
@@ -0,0 +1,41 @@
+############################################################################
+# apps/examples/mcuboot/update_agent/Makefile
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership. The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+include $(APPDIR)/Make.defs
+
+ifneq ($(CONFIG_EXAMPLES_MCUBOOT_UPDATE_AGENT),)
+MAINSRC += mcuboot_agent_main.c
+
+PROGNAME += mcuboot_agent
+PRIORITY += SCHED_PRIORITY_DEFAULT
+STACKSIZE += $(CONFIG_DEFAULT_TASK_STACKSIZE)
+endif
+
+ifneq ($(CONFIG_EXAMPLES_MCUBOOT_UPDATE_AGENT_SLOT_CONFIRM),)
+MAINSRC += mcuboot_confirm_main.c
+
+PROGNAME += mcuboot_confirm
+PRIORITY += SCHED_PRIORITY_DEFAULT
+STACKSIZE += $(CONFIG_DEFAULT_TASK_STACKSIZE)
+endif
Review comment:
The Update Agent and the Confirm are two separate examples.
They should not be considered as a single `update_agent` example inside `examples/mcuboot`.
I'd suggest one of these two approaches:
- Either simply move the examples to the `examples/mcuboot`, without the `update_agent` folder (**preferred**)
- Or separate them, each one into its own folder (e.g. `examples/mcuboot/update_agent` and `examples/mcuboot/confirm`).
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r773104142
##########
File path: boot/mcuboot/README.md
##########
@@ -22,9 +22,7 @@ The NuttX port of MCUboot is implemented at application-level and requires minim
## Creating MCUboot-compatible application firmware images
-One common use case for MCUboot is to integrate it to a firmware update agent, which is an important component of a secure firmware update subsystem. Through MCUboot APIs an application is able to install a newly received application firmware image and, once this application firmware image is assured to be valid, the application may confirm it as a stable image. In case that application firmware image is deemed bogus, MCUboot provides an API for invalidating that update, which will induce a rollback procedure to the most recent stable application firmware image.
-
-The `CONFIG_MCUBOOT_UPDATE_AGENT_EXAMPLE` example demonstrates this workflow by downloading an application firmware image from a webserver, installing it and triggering the firmware update process for the next boot after a system reset. There is also the `CONFIG_MCUBOOT_SLOT_CONFIRM_EXAMPLE`, which is a fairly simple example that just calls an MCUboot API for confirming the executing application firmware image as stable.
+See `examples/mcuboot` examples.
Review comment:
I am okay with replacing the second paragraph with this pointer to the examples folder, but why remove the first paragraph?
That documentation has no relation to the any of the examples.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r773125860
##########
File path: boot/mcuboot/README.md
##########
@@ -22,9 +22,7 @@ The NuttX port of MCUboot is implemented at application-level and requires minim
## Creating MCUboot-compatible application firmware images
-One common use case for MCUboot is to integrate it to a firmware update agent, which is an important component of a secure firmware update subsystem. Through MCUboot APIs an application is able to install a newly received application firmware image and, once this application firmware image is assured to be valid, the application may confirm it as a stable image. In case that application firmware image is deemed bogus, MCUboot provides an API for invalidating that update, which will induce a rollback procedure to the most recent stable application firmware image.
-
-The `CONFIG_MCUBOOT_UPDATE_AGENT_EXAMPLE` example demonstrates this workflow by downloading an application firmware image from a webserver, installing it and triggering the firmware update process for the next boot after a system reset. There is also the `CONFIG_MCUBOOT_SLOT_CONFIRM_EXAMPLE`, which is a fairly simple example that just calls an MCUboot API for confirming the executing application firmware image as stable.
+See `examples/mcuboot` examples.
Review comment:
```suggestion
One common use case for MCUboot is to integrate it to a firmware update agent, which is an important component of a secure firmware update subsystem. Through MCUboot APIs an application is able to install a newly received application firmware image and, once this application firmware image is assured to be valid, the application may confirm it as a stable image. In case that application firmware image is deemed bogus, MCUboot provides an API for invalidating that update, which will induce a rollback procedure to the most recent stable application firmware image.
The `CONFIG_EXAMPLES_MCUBOOT_UPDATE_AGENT` example demonstrates this workflow by downloading an application firmware image from a webserver, installing it and triggering the firmware update process for the next boot after a system reset. There is also the `CONFIG_EXAMPLES_MCUBOOT_SLOT_CONFIRM`, which is a fairly simple example that just calls an MCUboot API for confirming the executing application firmware image as stable.
```
Actually, since the examples still exist, I see no reason to remove it. Just update the reference to the Kconfig keys.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r773126229
##########
File path: examples/mcuboot/update_agent/Kconfig
##########
@@ -0,0 +1,44 @@
+#
+# For a description of the syntax of this configuration file,
+# see the file kconfig-language.txt in the NuttX tools repository.
+#
+
+config EXAMPLES_MCUBOOT_UPDATE_AGENT
+ bool "MCUboot update agent example"
+ default n
+ depends on NET_TCP
+ ---help---
+ Example application that implements an update agent that downloads
+ an application firmware image from a given URL and saves it to the
+ secondary slot as a pending update.
+
+if EXAMPLES_MCUBOOT_UPDATE_AGENT
+
+config EXAMPLES_MCUBOOT_UPDATE_AGENT_UPDATE_URL
+ string "URL for update image"
+ default ""
+
+config EXAMPLES_MCUBOOT_UPDATE_AGENT_DL_BUFFER_SIZE
+ int "Download buffer size in bytes"
+ default 512
+
+config EXAMPLES_MCUBOOT_UPDATE_AGENT_DL_VERIFY_MD5
+ bool "Calculate MD5 of update image"
+ default n
+ depends on CODECS_HASH_MD5
+
+config EXAMPLES_MCUBOOT_UPDATE_AGENT_DL_MD5_HASH
+ string "Expected MD5 sum of update image"
+ default ""
+ depends on EXAMPLES_MCUBOOT_UPDATE_AGENT_DL_VERIFY_MD5
+
+config EXAMPLES_MCUBOOT_UPDATE_AGENT_SLOT_CONFIRM
+ tristate "MCUboot slot confirm example"
+ default n
+ ---help---
+ Example application for confirming a newly installed application
+ application firmware image using MCUboot public APIs.
+ This application should be used as the OTA update package of the
+ EXAMPLES_MCUBOOT_UPDATE_AGENT example.
Review comment:
This should be moved as well to a dedicated folder for the `confirm` example.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 edited a comment on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1057018989
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] otavio commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
otavio commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1057048343
@gustavonihei, as O.S. Systems, we have been sponsoring the work on NuttX in this area, and this lost time and energy doesn't make sense. Furthermore, it causes contributors (and companies) to lose interest as we cannot keep walking in circles without a clear direction or benefit in the long term.
In the absence of guidelines, the code owner (subproject or component) should be giving the final word about the design. Who is the code owner of this component?
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1059253203
How to proceed this PR now? It isn't good to hold a PR for long time.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] nandojve commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
nandojve commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1063958346
> Look like no more objection, @nandojve could you run refresh.sh --silent for the follow config to fix the CI fail:
>
> ```
> modified: boards/arm/samv7/same70-qmtech/configs/mcuboot-confirm/defconfig
> modified: boards/arm/samv7/same70-xplained/configs/mcuboot-confirm/defconfig
> modified: boards/arm/samv7/samv71-xult/configs/mcuboot-nsh/defconfig
> ```
>
> https://github.com/apache/incubator-nuttx-apps/runs/5344564439?check_suite_focus=true So, I can merge both change.
Hi @xiaoxiang781216 ,
I'll do soon, thks!
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] nandojve commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
nandojve commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1072438065
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] pkarashchenko commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1045258368
> > @nandojve how about this patch?
>
> Indeed, this is taking more time than expected. I think I can recap it in one maybe two weeks.
>
> BTW, I noted that there is not available option to move to draft. That could be helpful in this kind of situation because I could just move to draft and when it is ready again move to open.
There is an option "move to draft" it is just hidden well by GitHub UI :)
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei edited a comment on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1057002869
> @gustavonihei I think @nandojve is a valid concern. However, this cannot be based on personal feelings but on a general project guideline that must be documented somewhere to avoid this.
@otavio Sure, I completely agree with this.
I am sorry if I was not completely clear in my previous message, but I never prioritize personal preferences on my review activities. And if for some reason I need to add personal preferences to the review, I will proactively add this disclaimer.
> A project as mature and big as NuttX should have this somewhere, so where is the guideline?
As far as I know, there is no guideline. At least I am not aware of it.
And, as I stated in my previous comment, in the absence of documented guidelines, it all narrows down to personal preference.
Anyone is free and much welcome to contribute to the project documentation and improve it.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] Ouss4 commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1057265397
> In the absence of guidelines, the code owner (subproject or component) should be giving the final word about the design. Who is the code owner of this component?
In Apache projects no single individual owns a component or part of the project. The project is managed by the (P)PMC but the whole community participates in its development. When a matter is up for discussion, it's preferred to reach a consensus than having a single individual giving the final word.
Of course some people are more experienced/familiar with parts of the project than others and they review or are asked to review these parts. But the review or the approval doesn't always come from committers or members of the PMC, anyone in the community is welcomed to help with this.
Regarding guidelines to propose new features/design, the usual DISCUSS/VOTE can be used:
1. Opening a DISCUSS thread in the mailing list explaining the proposal. This last for 72 hours.
2. After the DISCUSS thread, a VOTE thread can be opened for 72 hours to have the final decision.
When it comes to adding features we only had to go through this process a few times. It's simpler and quicker to just explain the motivations in the PR discussion with the people involved. However, when it's hard to reach consensus, then it would be better to ask for the opinion of the whole community.
Please share your concerns on what you think can be made clearer to help people contributing and proposing new features.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1063617630
Look like no more objection, @nandojve could you run refresh.sh --silent for the follow config to fix the CI fail:
modified: boards/arm/samv7/same70-qmtech/configs/mcuboot-confirm/defconfig
modified: boards/arm/samv7/same70-xplained/configs/mcuboot-confirm/defconfig
modified: boards/arm/samv7/samv71-xult/configs/mcuboot-nsh/defconfig
So, I can merge both change.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r772687373
##########
File path: examples/mcuboot/ota/Kconfig
##########
@@ -0,0 +1,56 @@
+#
+# For a description of the syntax of this configuration file,
+# see the file kconfig-language.txt in the NuttX tools repository.
+#
+
+menuconfig EXAMPLES_MCUBOOT_OTA
Review comment:
Sorry, I missed one point that you commented earlier which is important to the discussion:
> The suffix APP_AGENT + APP_CONFIRM are enough to represent each application inside this demo.
Although this demo you are proposing includes both example applications, I believe there is no need to reflect it in the folder structure or in the kconfig hierarchy. The demo is best characterized by the set of options included in the `defconfig` file.
So all the MCUboot example application should appear on the level at `examples/mcuboot`.
##########
File path: examples/mcuboot/ota/Kconfig
##########
@@ -0,0 +1,56 @@
+#
+# For a description of the syntax of this configuration file,
+# see the file kconfig-language.txt in the NuttX tools repository.
+#
+
+menuconfig EXAMPLES_MCUBOOT_OTA
Review comment:
Sorry, I missed one point that you commented earlier which is important to the discussion:
> The suffix APP_AGENT + APP_CONFIRM are enough to represent each application inside this demo.
Although this demo you are proposing includes both example applications, I believe there is no need to reflect it in the folder structure or in the kconfig hierarchy. The demo is best characterized by the set of options included in the `defconfig` file.
So all the MCUboot example applications should appear on the level at `examples/mcuboot`.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] nandojve commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
nandojve commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r772372999
##########
File path: examples/mcuboot_ota/Kconfig
##########
@@ -0,0 +1,63 @@
+#
+# For a description of the syntax of this configuration file,
+# see the file kconfig-language.txt in the NuttX tools repository.
+#
+
+menuconfig EXAMPLES_MCUBOOT_OTA
+ tristate "MCUboot OTA example"
+ default n
+ select BOOT_MCUBOOT
+ ---help---
+ Example application demonstrate a software firmware update.
+ It downloads an application firmware image from a given URL.
+
+if EXAMPLES_MCUBOOT_OTA
+
+comment "This example requires network"
+ depends on !NET_TCP
+
+choice EXAMPLES_MCUBOOT_OTA_APP
+ prompt "Select MCUboot application"
+ default EXAMPLES_MCUBOOT_OTA_APP_AGENT
+ depends on NET_TCP
+ ---help---
+ Select MCUboot application.
+
+config EXAMPLES_MCUBOOT_OTA_APP_AGENT
+ bool "Agent"
+ ---help---
+ Implements the update agent that downloads the application
+ firmware image from a given URL and saves it to the secondary
+ slot as a pending update.
+
+config EXAMPLES_MCUBOOT_OTA_APP_CONFIRM
+ bool "Confirm"
+ ---help---
+ Is the application to be download to demonstrate firmware
+ upgrade functionality.
+
+endchoice
Review comment:
This example require two steps: one is the download agent, second is the app that will be used to demonstrate that application (agent) was upgraded, which it should be different at some aspect to achieve that. My understanding is that it require two different build.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] nandojve commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
nandojve commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1045161453
> @nandojve how about this patch?
Indeed, this is taking more time than expected. I think I can recap it in one maybe two weeks.
BTW, I noted that there is not available option to move to draft. That could be helpful in this kind of situation because I could just move to draft and when it is ready again move to open.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei edited a comment on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1057002869
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 edited a comment on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1057018989
I personally follow this order:
1. If this program is general enough to be used in many case without the code modification, it's better to move out of apps/examples folder.
2. Otherwise, it's better to put into the corresponding subsystem folder to indicate that it's very mature and ready for use immediately.
Since many application doesn't follow this and put into the source tree randomly, I am fine with the change.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] pkarashchenko merged pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r772352237
##########
File path: examples/mcuboot_ota/Kconfig
##########
@@ -0,0 +1,63 @@
+#
+# For a description of the syntax of this configuration file,
+# see the file kconfig-language.txt in the NuttX tools repository.
+#
+
+menuconfig EXAMPLES_MCUBOOT_OTA
+ tristate "MCUboot OTA example"
+ default n
+ select BOOT_MCUBOOT
+ ---help---
+ Example application demonstrate a software firmware update.
+ It downloads an application firmware image from a given URL.
+
+if EXAMPLES_MCUBOOT_OTA
+
+comment "This example requires network"
+ depends on !NET_TCP
+
+choice EXAMPLES_MCUBOOT_OTA_APP
+ prompt "Select MCUboot application"
+ default EXAMPLES_MCUBOOT_OTA_APP_AGENT
+ depends on NET_TCP
+ ---help---
+ Select MCUboot application.
+
+config EXAMPLES_MCUBOOT_OTA_APP_AGENT
+ bool "Agent"
+ ---help---
+ Implements the update agent that downloads the application
+ firmware image from a given URL and saves it to the secondary
+ slot as a pending update.
+
+config EXAMPLES_MCUBOOT_OTA_APP_CONFIRM
+ bool "Confirm"
+ ---help---
+ Is the application to be download to demonstrate firmware
+ upgrade functionality.
+
+endchoice
Review comment:
What's the reason for making this examples mutually-exclusive?
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r772550909
##########
File path: examples/mcuboot/ota/Kconfig
##########
@@ -0,0 +1,56 @@
+#
+# For a description of the syntax of this configuration file,
+# see the file kconfig-language.txt in the NuttX tools repository.
+#
+
+menuconfig EXAMPLES_MCUBOOT_OTA
Review comment:
What's the reason for changing the application name?
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r773126229
##########
File path: examples/mcuboot/update_agent/Kconfig
##########
@@ -0,0 +1,44 @@
+#
+# For a description of the syntax of this configuration file,
+# see the file kconfig-language.txt in the NuttX tools repository.
+#
+
+config EXAMPLES_MCUBOOT_UPDATE_AGENT
+ bool "MCUboot update agent example"
+ default n
+ depends on NET_TCP
+ ---help---
+ Example application that implements an update agent that downloads
+ an application firmware image from a given URL and saves it to the
+ secondary slot as a pending update.
+
+if EXAMPLES_MCUBOOT_UPDATE_AGENT
+
+config EXAMPLES_MCUBOOT_UPDATE_AGENT_UPDATE_URL
+ string "URL for update image"
+ default ""
+
+config EXAMPLES_MCUBOOT_UPDATE_AGENT_DL_BUFFER_SIZE
+ int "Download buffer size in bytes"
+ default 512
+
+config EXAMPLES_MCUBOOT_UPDATE_AGENT_DL_VERIFY_MD5
+ bool "Calculate MD5 of update image"
+ default n
+ depends on CODECS_HASH_MD5
+
+config EXAMPLES_MCUBOOT_UPDATE_AGENT_DL_MD5_HASH
+ string "Expected MD5 sum of update image"
+ default ""
+ depends on EXAMPLES_MCUBOOT_UPDATE_AGENT_DL_VERIFY_MD5
+
+config EXAMPLES_MCUBOOT_UPDATE_AGENT_SLOT_CONFIRM
+ tristate "MCUboot slot confirm example"
+ default n
+ ---help---
+ Example application for confirming a newly installed application
+ application firmware image using MCUboot public APIs.
+ This application should be used as the OTA update package of the
+ EXAMPLES_MCUBOOT_UPDATE_AGENT example.
Review comment:
Refer to https://github.com/apache/incubator-nuttx-apps/pull/929/files#r773105477
The `Update Agent` and the `Confirm` example have no coupling between them and should not be considered a single example.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r772587297
##########
File path: examples/mcuboot/ota/Kconfig
##########
@@ -0,0 +1,56 @@
+#
+# For a description of the syntax of this configuration file,
+# see the file kconfig-language.txt in the NuttX tools repository.
+#
+
+menuconfig EXAMPLES_MCUBOOT_OTA
Review comment:
In fact, I'd suggest for this PR to just restrict the scope to moving the contents without performing further changes. These minor changes are difficult to track, since GitHub simply identifies the destination as a new file.
Then, in a separate PR, perform the changes with proper motivation for them. Most of the changes are superfluous in my opinion, so the discussion over them could hinder the PR getting merged.
Edit: of course, the "EXAMPLES_" prefix should still be added due to new folder structure.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r772587297
##########
File path: examples/mcuboot/ota/Kconfig
##########
@@ -0,0 +1,56 @@
+#
+# For a description of the syntax of this configuration file,
+# see the file kconfig-language.txt in the NuttX tools repository.
+#
+
+menuconfig EXAMPLES_MCUBOOT_OTA
Review comment:
In fact, I'd suggest for this PR to just restrict the scope to moving the contents without performing further changes. These minor changes are difficult to track, since GitHub simply identifies the destination as a new file.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei edited a comment on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1056941297
> Hi Folks,
>
> This change of feelings is that create bad projects. I invested a lot of time to put things in the correct way and, after weeks, now, it is about feelings. Sorry, but this is very disrespectful.
>
I understand that you invested time in this PR and that you may feel it is being undervalued.
This PR has been open for months, but reviewed just by a single person (me).
My feelings about moving the examples to the `examples` folder should not be new to you @nandojve, as we've discussed several times before, so on my side there is no "change of feelings". If I really agreed to this, I would have already done this way from the very beginning. I still feel that the examples from a given library should be placed in the same level.
Even so, I haven't complained about this in this PR. My expectation was to see if the other developers were OK with it, without adding any bias.
The points I raised on this PR is to prevent further deviations from the project structure, such as the naming of the Kconfig options. So, in case anyone approved it, at least it is being done in the most correct way.
> If people from NuttX detected a issue about where examples should be added, that should be documented (no more feelings), that it. Since the issue was detected in this PR, and PR was complete reworked to be as requested, at least, you folks should respect the work done and approve it.
I am responsible for the code I approve to be merged to the project's codebase. And so far I am still not confident in doing so, according to the reason I stated on the previous message.
Let's wait on the feedback from others.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1056941297
> Hi Folks,
>
> This change of feelings is that create bad projects. I invested a lot of time to put things in the correct way and, after weeks, now, it is about feelings. Sorry, but this is very disrespectful.
>
I understand that you invested time in this PR and that you may feel it is being undervalued.
This PR has been open for months, but reviewed just by a single person (me).
My feelings about moving the examples to the `examples` folder should not be new to you @nandojve, as we've discussed several times before, so on my side there is no "change of feelings". If I really agreed to this, I would have already done this way from the very beginning. I still feel that the examples from a given library should be placed in the same level.
Even so, I haven't complained about this in this PR. My expectation was to see if the other developers were OK with it, without adding any bias.
The points I raised on this PR is to prevent further deviations from the project structure, such as the naming of the Kconfig options.
> If people from NuttX detected a issue about where examples should be added, that should be documented (no more feelings), that it. Since the issue was detected in this PR, and PR was complete reworked to be as requested, at least, you folks should respect the work done and approve it.
I am responsible for the code I approve to be merged to the project's codebase. And so far I am still not confident in doing so, according to the reason I stated on the previous message.
Let's wait on the feedback from others.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei edited a comment on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1057002869
> @gustavonihei I think @nandojve is a valid concern. However, this cannot be based on personal feelings but on a general project guideline that must be documented somewhere to avoid this.
@otavio Sure, I completely agree with this.
I am sorry if I was not completely clear in my previous message, but I never prioritize personal preferences on my review activities. And if for some reason I need to add personal preferences to the review, I will proactively add this disclaimer.
> A project as mature and big as NuttX should have this somewhere, so where is the guideline?
As far as I know, there is no guideline. At least I am not aware of it.
And, as I stated in my previous comment, in the absence of documented guidelines, it all narrows down to personal preference.
Anyone is free and much welcome to contribute to the project documentation and improve it.
I am not confident in approving a PR without a consistent motivation.
At the same time, I will not reject it simply because I do not like or agree with it.
@otavio @nandojve I hope you understand my position.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei edited a comment on pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#issuecomment-1057002869
> @gustavonihei I think @nandojve is a valid concern. However, this cannot be based on personal feelings but on a general project guideline that must be documented somewhere to avoid this.
@otavio Sure, I completely agree with this.
I am sorry if I was not completely clear in my previous message, but I never prioritize personal preferences on my review activities. And if for some reason I do need to add personal preferences to the review, I will proactively add this disclaimer.
> A project as mature and big as NuttX should have this somewhere, so where is the guideline?
As far as I know, there is no guideline. At least I am not aware of it.
And, as I stated in my previous comment, in the absence of documented guidelines, it all narrows down to personal preference.
Anyone is free and much welcome to contribute to the project documentation and improve it.
I am not confident in approving a PR without a consistent motivation.
At the same time, I will not reject it simply because I do not like or agree with it.
@otavio @nandojve I hope you understand my position.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] nandojve commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
nandojve commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r772583612
##########
File path: examples/mcuboot/ota/Kconfig
##########
@@ -0,0 +1,56 @@
+#
+# For a description of the syntax of this configuration file,
+# see the file kconfig-language.txt in the NuttX tools repository.
+#
+
+menuconfig EXAMPLES_MCUBOOT_OTA
Review comment:
Originally those are two apps inside boot/mcuboot with two distinct names that makes sense inside that scope (MCUBOOT_UPDATE_AGENT_EXAMPLE + MCUBOOT_SLOT_CONFIRM_EXAMPLE).
With this refactor prefix changed to `EXAMPLES_MCUBOOT_OTA` which represents examples/mcuboot/ota were OTA became the example name, which gave more sense of consistency. The suffix APP_AGENT + APP_CONFIRM are enough to represent each application inside this demo.
Do you prefer `EXAMPLES_MCUBOOT_OTA_UPDATE_AGENT` and `EXAMPLES_MCUBOOT_OTA_SLOT_CONFIRM` ?
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] nandojve commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
nandojve commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r772653518
##########
File path: examples/mcuboot/ota/Kconfig
##########
@@ -0,0 +1,56 @@
+#
+# For a description of the syntax of this configuration file,
+# see the file kconfig-language.txt in the NuttX tools repository.
+#
+
+menuconfig EXAMPLES_MCUBOOT_OTA
Review comment:
Ok, good. It is necessary define a name for this App. I proposed `ota`, which seems not be a good one. So, I'm asking, what in your opinion should be a good App name for this App?
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #929: MCUboot move apps to examples
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #929:
URL: https://github.com/apache/incubator-nuttx-apps/pull/929#discussion_r772657189
##########
File path: examples/mcuboot/ota/Kconfig
##########
@@ -0,0 +1,56 @@
+#
+# For a description of the syntax of this configuration file,
+# see the file kconfig-language.txt in the NuttX tools repository.
+#
+
+menuconfig EXAMPLES_MCUBOOT_OTA
Review comment:
First: Why Update Agent is not a good name for the app?
It is the current name for the app. There should be a reason for changing it.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org