boards: Fix modbus configs after moving it to apps/industry#18744
boards: Fix modbus configs after moving it to apps/industry#18744simbit18 merged 1 commit intoapache:masterfrom
Conversation
acassis
requested review from
Donny9,
GUIDINGLI,
rcsim and
xiaoxiang781216
as code owners
github-actions
Bot
added
Size: S
|
It is funny, I was going to update the Documentation and discovery that is was already appointing to apps/industry/modbus: applications/examples/modbus/index.rst No idea how it happened :-D |
Actually it was @raiden00pl who predicted that it was going to happen! :-D |
|
@acassis I've had this on my TODO list for a long time :) 
|
|
Hmm, I made a mistake: I renamed the CONFIG_INDUSTRY_MODBUS directly inside defconfig,:
|
|
Hmm, I am wondering if Instead, if we plan to have different modbus implementations we could use something like For other implementations we would then use |
|
The problem with the current Kconfig is that we don't have any standardization for option names. Some programs/libs use one format, others another. The main (and probably the only disadvantage of INDUSTRY_MODBUS) is that the options become long, the advantage is that we have context added to the option name (like INDUSTRY). Personally, I would be in favor of the name |
@cederom keeping the INDUSTRY in the name is important to context, someone seem CONFIG_INDUSTRY_MODBUS will know he/she needs to look inside apps/industry/ to find it. Same is used in many places like apps/graphics, apps/testing, etc. In same places part of the sub-directories are suppressed (i.e. apps/testing/fs/smart) I think if we find places where this convention is not used we need to fix it. But of course as @raiden00pl noticed, we need to avoid long names. |
|
Up to you guys, just thinking aloud, fine for me with the industry included, but it may be good to name it freemodbus though if we plan differen modbus implementations? Thanks! :-) |
|
@simbit18 @lupyuen any idea why the defconfig says it is not normalize, at least here I can see it is normalize: |
Thank you @simbit18, but the original CI shouldn't report an error if the board are synced/normalized. |
|
@acassis I tested with Docker, refresh.sh seems to be working correctly: https://gist.github.com/lupyuen/a77be35dbcd40f959e2e4930d126963b $ sudo docker run -it \
ghcr.io/apache/nuttx/apache-nuttx-ci-linux:latest
# cd ; pwd ;
# git clone https://github.com/acassis/incubator-nuttx --branch fix_modbus_loc nuttx
# git clone https://github.com/apache/nuttx-apps apps ;
# cd nuttx
# ./tools/refresh.sh --silent stm32f4discovery:modbus_slave
[1/1] Normalize stm32f4discovery:modbus_slave
9d8
< # CONFIG_MB_TCP_ENABLED is not set
32,33d30
< CONFIG_INDUSTRY_MODBUS=y
< CONFIG_INDUSTRY_MODBUS_SLAVE=y
Saving the new configuration file
# git diff
diff --git a/boards/arm/stm32/stm32f4discovery/configs/modbus_slave/defconfig b/boards/arm/stm32/
stm32f4discovery/configs/modbus_slave/defconfig
index 0eba5c05bc..e574e4de0b 100644
--- a/boards/arm/stm32/stm32f4discovery/configs/modbus_slave/defconfig
+++ b/boards/arm/stm32/stm32f4discovery/configs/modbus_slave/defconfig
@@ -6,7 +6,6 @@
# modifications.
#
# CONFIG_ARCH_FPU is not set
-# CONFIG_MB_TCP_ENABLED is not set
# CONFIG_NSH_ARGCAT is not set
# CONFIG_NSH_CMDOPT_HEXDUMP is not set
# CONFIG_NSH_DISABLE_MB is not set
@@ -29,8 +28,6 @@ CONFIG_EXAMPLES_MODBUS_PORT=1
CONFIG_FS_PROCFS=y
CONFIG_HAVE_CXX=y
CONFIG_HAVE_CXXINITIALIZE=y
-CONFIG_INDUSTRY_MODBUS=y
-CONFIG_INDUSTRY_MODBUS_SLAVE=y
CONFIG_INIT_ENTRYPOINT="nsh_main"
CONFIG_INTELHEX_BINARY=y
CONFIG_LINE_MAX=64Update: refresh.sh also works OK on macOS and Ubuntu. Is there something special about your PC?
Or maybe you're using a different NuttX Apps repo? git clone https://github.com/apache/nuttx-apps apps |
|
Hi @acassis, I hope to explain below why we came up with this new system and why I advised you to take a test.
The NuttX source code here includes the changes made to defconfig
The NuttX source here does not includes defconfig changes
The NuttX source code here includes the changes made to defconfig When a change affects the PRs in both repositories, it is very difficult for NuttX maintainers to check whether those changes might cause problems after merging. This new system helps us to verify that the build at least doesn’t break anything. Results of the test I carried out Linux (arm-13) |
|
Hi @acassis Have you tested this build linum-stm32h753bi:modbus_master? |
The modbus was moved to inside apps/industry, so these defconfigs need to be updated Signed-off-by: Alan C. Assis <acassis@gmail.com>
I tested again and fixed the issue, some Kconfigs were missing, thank you for your help! |
|
@acassis do we want the |
I think if in the future we get a new MODBUS library we can change the name. For now we only have a unique modbus lib. :-) |
whoa!!! Things happen fast on NuttX :-D |
cederom
left a comment
cederom
left a comment
There was a problem hiding this comment.
Okay, lets go with this one, next lets update to freemodbus as nxmodbus showed up for review here github.com/apache/nuttx-apps/pull/3459 :-)
cederom
left a comment
cederom
left a comment
There was a problem hiding this comment.
oops, some ci build fails at xtensa :-(
Strange, the config seems normalized: |
cederom
left a comment
cederom
left a comment
There was a problem hiding this comment.
Looks like CI is broken at Xtensa I have the same issue :-(
|
Hi @cederom, the job Linux (xtensa-01) fails here because
Since the Apache apps master doesn't contain the additional changes required, it rightly fails I tested it here https://github.com/simbit18/manual-nuttx-ci/actions/runs/24637035652 using this new #18568 tool and didn’t encounter any issues Set it up as follows: Workflow -> Manually build NuttX CI for test I'm merging this PR |
Thank you @simbit18 ! So just to config and let see if I'm in the same page: 1) now the CI will try to build using github of the user that submitted the PR; 2) Since I my PR was coming from a different branch, instead of master, the issue was happening. Is that the root cause of the issue? |
Hi @acassis No, that’s not the problem!! :) Your changes affected two repositories! To avoid workflow errors, you should have first merged at least one PR (on nuttx or apps) but with comprehensive tests covering all jobs! That’s why I recommend everyone uses this #18568 tool :
|
6 participants
Summary
The modbus was moved to inside apps/industry, so these defconfigs need to be updated
Impact
None, only organization
Testing
On NuttX board:
On Linux side: