modbus: Respect CONFIG_UART_USE_RUNTIME_CONFIGURE flag #68858

zagor · 2024-02-12T10:34:35Z

Only perform runtime UART configuration if it is enabled.

This allows using serial modbus on a native ptty.

zagor · 2024-02-12T10:37:07Z

This together with #68857 enables testing modbus communication on native_posix builds.

Edit: More precisely, it enables testing modbus communication between two native_posix builds without any hardware attached. Or between one native_posix zephyr application and some other software speaking modbus.

I do this by creating two pseudo-ttys with the command socat pty,rawer,link=/tmp/ttyA pty,rawer,link=/tmp/ttyB and then passing them to the zephyr applications using the -uart_port argument. I feel this solution would be good to document somewhere, but I am unsure what is a good location for it.

jfischer-no · 2024-02-15T10:54:41Z

subsys/modbus/modbus_serial.c

+#ifdef CONFIG_UART_USE_RUNTIME_CONFIGURE
+	struct uart_config uart_cfg;


CONFIG_UART_USE_RUNTIME_CONFIGURE is a Kconfig option not a flag.
Runtime configuration is mandatory for Modbus with no exceptions. You have to implement it where it is missed.

Why do you want runtime configuration to be mandatory for modbus?

Because is is designed to use runtime configuration to e.g. update server baudrate. And there is a dependency on 'UART_USE_RUNTIME_CONFIGURE' in the Kconfig file. And based on your motivation, it must be implemented for native_posix/native_sim to do "testing modbus". Otherwise, you are not testing exactly what the test is supposed to test. Finally, I personally see no reason to have another Kconfig option.

Just because a use case for runtime configuration exists doesn't mean it is mandatory. We have a Kconfig option to give users the power to choose if they want this or not. What gives you the right to override their choice, forcing them to include code they have explicitly said they do not want? It adds 1 KB to my STM32 build.

I run virtual system tests involving multiple zephyr native_sim applications that talk to each other using modbus. I don't want to "test modbus", I want to test my application logic.

Just because a use case for runtime configuration exists doesn't mean it is mandatory.

It is also because "there is a dependency on 'UART_USE_RUNTIME_CONFIGURE' in the Kconfig file" and you patch does not do what you think.

We have a Kconfig option to give users the power to choose if they want this or not. What gives you the right to override their choice, forcing them to include code they have explicitly said they do not want?

Maybe my role as author and maintainer gives me the right to drive it and decide.
But I do not force anyone "to include code they have explicitly said they do not want". The system has this dependency either you fulfill this dependency or it does not work. Btw, Kconfig option UART_USE_RUNTIME_CONFIGURE was introduced after Modbus subsys, and other effective changes in the drivers even later.

It adds 1 KB to my STM32 build.

Great, glad to hear it does not take that much flash. However that was not the motivation according to the commit message.

I run virtual system tests involving multiple zephyr native_sim applications that talk to each other using modbus. I don't want to "test modbus", I want to test my application logic.

This is also understandable yet. If you have implemented it for native_posix/native_sim, nothing stands in the way, right?

But I do not force anyone "to include code they have explicitly said they do not want". The system has this dependency either you fulfill this dependency or it does not work.

You say that, yet the modbus subsystem works just fine without it. Where is this hard dependency?

Kconfig option UART_USE_RUNTIME_CONFIGURE was introduced after Modbus subsys, and other effective changes in the drivers even later.

That may explain why it wasn't already added. It does not explain why you won't let anyone add it now.

It adds 1 KB to my STM32 build.

Great, glad to hear it does not take that much flash.

I don't think it is up to you or me to decide what users' code space is worth and what they should use it for. We can safely assume that anyone who chooses to disable UART_USE_RUNTIME_CONFIGURE considers the size saving to be significant.

But I do not force anyone "to include code they have explicitly said they do not want". The system has this dependency either you fulfill this dependency or it does not work.

You say that, yet the modbus subsystem works just fine without it. Where is this hard dependency?

Original description as motivation for your change:
"This together with #68857 enables testing modbus communication on native_posix builds."
In the context of this project, it means being able to test tests/subsys/modbus, and requires UART driver to implement uart_configure(). Your patch had no effect because of "select UART_USE_RUNTIME_CONFIGURE". Subsys is designed to configure and reconfigure interfaces at runtime. Your changes were far from what would be reasonable or acceptable.

Kconfig option UART_USE_RUNTIME_CONFIGURE was introduced after Modbus subsys, and other effective changes in the drivers even later.

That may explain why it wasn't already added. It does not explain why you won't let anyone add it now.

Because these changes are not necessary and and smaller additional consumption of resources is acceptable.

zagor · 2024-02-16T08:35:45Z

Updated patch with the missed Kconfig change.

subsys/modbus/Kconfig

jfischer-no · 2024-02-16T09:50:30Z

subsys/modbus/modbus_serial.c

@@ -521,6 +520,9 @@ int modbus_serial_init(struct modbus_context *ctx,
 		return -ENODEV;
 	}

+#ifdef CONFIG_UART_USE_RUNTIME_CONFIGURE


This can and should be solved differently, without ifdefery.

zagor · 2024-02-21T09:22:27Z

Latest patch addresses all technical concerns and passes all checks. What more is required to merge this?

zagor · 2024-03-07T13:02:11Z

I'll mention this note by @nordic-krch in the Zephyr 3.6 migration notes that is relevant to this PR:

Runtime configuration is now disabled by default for Nordic UART drivers. The motivation for the change is that this feature is rarely used and disabling it significantly reduces the memory footprint.

jfischer-no

Not addressed https://github.com/zephyrproject-rtos/zephyr/pull/68858/files#r1492212243

zagor · 2024-03-07T20:39:16Z

Not addressed https://github.com/zephyrproject-rtos/zephyr/pull/68858/files#r1492212243

It is addressed, using the solution you yourself pointed to. If you are not happy with imply, what would you prefer?

jfischer-no · 2024-03-07T21:31:48Z

Not addressed https://github.com/zephyrproject-rtos/zephyr/pull/68858/files#r1492212243

It is addressed, using the solution you yourself pointed to. If you are not happy with imply, what would you prefer?

right, my fault, this one #68858 (comment)

Here is the patch https://github.com/jfischer-no/zephyr/commit/8727703a1a9121eb5d6c97358e4fd6f503bc7638.patch, sorry, was faster for me to write it than to try to explain it and complain about the commit message.

zagor · 2024-03-08T08:13:41Z

Here is the patch https://github.com/jfischer-no/zephyr/commit/8727703a1a9121eb5d6c97358e4fd6f503bc7638.patch

Sure, that's fine with me. But are you sure this is the solution you want? I chose the "ifdef empty function" solution to follow the code style of your implementation of modbus_ascii_rx_adu and modbus_ascii_tx_adu. I see no use of the "inline if IS_ENABLED" solution elsewhere in the modbus subsystem.

zagor · 2024-03-12T13:48:14Z

Rewritten as requested and rebased on main.

bjarki-andreasen

My two cents :)

bjarki-andreasen · 2024-03-13T03:58:27Z

subsys/modbus/Kconfig

@@ -44,7 +44,7 @@ config MODBUS_SERIAL
 	default y
 	depends on SERIAL && SERIAL_HAS_DRIVER
 	depends on DT_HAS_ZEPHYR_MODBUS_SERIAL_ENABLED
-	select UART_USE_RUNTIME_CONFIGURE
+	imply UART_USE_RUNTIME_CONFIGURE


Hi, I would remove this select/imply entirely :) The default value of UART_USE_RUNTIME_CONFIGURE is y, so unless the user explicitly adds CONFIG_UART_USE_RUNTIME_CONFIGURE=n to their .prj somewhere, it will be selected. This also means that no current users of the modbus_serial.c have explicitly disabled runtime as it produces this Kconfig error to do so:

warning: UART_USE_RUNTIME_CONFIGURE (defined at drivers/serial/Kconfig:51) was assigned the value 'n' but got the value 'y'. See http://docs.zephyrproject.org/latest/kconfig.html#CONFIG_UART_USE_RUNTIME_CONFIGURE and/or look up UART_USE_RUNTIME_CONFIGURE in the menuconfig/guiconfig interface. The Application Development Primer, Setting Configuration Values, and Kconfig - Tips and Best Practices sections of the manual might be helpful too.

If it is possible to use this driver without UART_USE_RUNTIME_CONFIGURE as it is after this PR, it shall not be selected in Kconfig. select is explicitly used only when the dependency is "small" but obligatory :)

I agree. The first version of this PR removed the select line entirely, but Johann refused it.

@jfischer-no can we agree on removing the select not being a breaking change? or have I missed something? :)

jfischer-no · 2024-03-28T11:45:09Z

Rewritten as requested and rebased on main.

I am afraid it is still not "Rewritten as requested", see #68858 (comment). And, you commit message is wrong, there is nothing like CONFIG_UART_USE_RUNTIME_CONFIGURE to implement, it is just a Kconfig option. There is a link to the patch you could just apply and force push to your PR branch and we would be done.

zagor · 2024-03-29T13:43:51Z

I am afraid it is still not "Rewritten as requested", see #68858 (comment).

Would you, please please with a cherry on top, say what you don't like instead of saying "it is wrong" over and over? It is really tiresome and a fantastic waste of both our times.

And, you commit message is wrong, there is nothing like CONFIG_UART_USE_RUNTIME_CONFIGURE to implement, it is just a Kconfig option.

implement, verb: "to start using a plan or system"

"Implement" is the proper verb. But I can change it back to "respect" if you prefer.

There is a link to the patch you could just apply and force push to your PR branch and we would be done.

Of course I'm not blind-committing code from someone else in my name. I seriously hope you never either.

If you'd rather commit your own-written version of this change, please just go ahead instead of playing these innuendo games for weeks. The only reason this PR drags on is because you consistently refuse to engage in discussion or provide constructive feedback.

henrikbrixandersen · 2024-04-02T08:40:04Z

I have added this PR to the Architecture WG project in order for it to get some traction. CC: @carlescufi

jfischer-no · 2024-04-03T09:14:01Z

I have added this PR to the Architecture WG project in order for it to get some traction. CC: @carlescufi

And I just removed it because there is nothing about architecture and therefore this label makes no sense.

carlescufi · 2024-04-03T09:26:00Z

I have added this PR to the Architecture WG project in order for it to get some traction. CC: @carlescufi

And I just removed it because there is nothing about architecture and therefore this label makes no sense.

Technically though the Arch WG meeting can be used for PR disagreement escalation:
https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#pr-technical-escalation

The contributor is free to ask for this topic to be discussed there.

jfischer-no · 2024-04-03T10:21:51Z

I am afraid it is still not "Rewritten as requested", see #68858 (comment).

Would you, please please with a cherry on top, say what you don't like instead of saying "it is wrong" over and over? It is really tiresome and a fantastic waste of both our times.

I am very sorry that you are frustrated. I only used "wrong" once in this PR and in the patch is the alternative commit title, I have pointed this out for some time.

There is a link to the patch you could just apply and force push to your PR branch and we would be done.

Of course I'm not blind-committing code from someone else in my name. I seriously hope you never either.

If you'd rather commit your own-written version of this change, please just go ahead instead of playing these innuendo games for weeks. The only reason this PR drags on is because you consistently refuse to engage in discussion or provide constructive feedback.

I do not understand why you refuse to look closer at the patch and apply it. I intentionally used the patch link instead of the branch link to make it easier for you and me. You would see that it is based on your original commit, see the differences, and see my sign-off.

zagor · 2024-04-03T12:17:14Z

I am very sorry that you are frustrated.

Are you? You ignore direct questions, state your opinions as facts, dont tell us when your opinions then change, stay silent for weeks, and still to this day refuse to do the most fundamental task of code reviewing: Saying what part of the patch you disagree with and why.

Open source development is based on the concept of talking about code and proposed changes. Conversations between peers where we look at code and proposals from many angles and try to make every change as good as we can. We learn from each other and cooperate to improve our shared code.

You appear to be under the illusion that we can do software development without talking. That the job of maintainers is merely to give a roman thumbs up or down and be done with it. But such an approach loses nearly all of the benefits of the open source process and you end up as just a gatekeeper discouraging contribution.

Code gets improved, increment by increment. That's what we do. That's why we are here. And that includes the modbus subsystem. It has its' share of warts and shortcomings to address, just as all code does. You need to start getting comfortable talking about it.

I do not understand why you refuse to look closer at the patch and apply it.

Two reasons:

I want you to say what is wrong with my patch. No more games.
You don't need me to commit your code. You are the maintainer.

Only perform runtime UART configuration if it is enabled. Signed-off-by: Björn Stenberg <[email protected]>

No longer relevant

carlescufi · 2024-04-09T18:07:18Z

Architecture WG:

@zagor will reword the commit message
@jfischer-no will take another look

zephyrbot added the area: modbus label Feb 12, 2024

zephyrbot requested a review from jfischer-no February 12, 2024 10:35

zephyrbot assigned jfischer-no Feb 12, 2024

jfischer-no requested changes Feb 15, 2024

View reviewed changes

zagor force-pushed the modbus-without-runtime-configure branch from 09b3729 to 5931028 Compare February 16, 2024 08:33

jfischer-no requested changes Feb 16, 2024

View reviewed changes

zagor force-pushed the modbus-without-runtime-configure branch from 5931028 to c73f505 Compare February 16, 2024 12:58

nordicjm added the dev-review To be discussed in dev-review meeting label Mar 7, 2024

jfischer-no requested changes Mar 7, 2024

View reviewed changes

jfischer-no removed the dev-review To be discussed in dev-review meeting label Mar 7, 2024

zagor force-pushed the modbus-without-runtime-configure branch from c73f505 to 38e310d Compare March 12, 2024 13:47

bjarki-andreasen previously requested changes Mar 13, 2024

View reviewed changes

henrikbrixandersen added the dev-review To be discussed in dev-review meeting label Mar 26, 2024

henrikbrixandersen added Architecture Review Discussion in the Architecture WG required and removed dev-review To be discussed in dev-review meeting labels Apr 2, 2024

henrikbrixandersen self-requested a review April 2, 2024 08:39

jfischer-no removed the Architecture Review Discussion in the Architecture WG required label Apr 3, 2024

decsny previously approved these changes Apr 9, 2024

View reviewed changes

modbus: Respect CONFIG_UART_USE_RUNTIME_CONFIGURE

81a3a0e

Only perform runtime UART configuration if it is enabled. Signed-off-by: Björn Stenberg <[email protected]>

zagor dismissed decsny’s stale review via 81a3a0e April 9, 2024 15:58

zagor force-pushed the modbus-without-runtime-configure branch from 38e310d to 81a3a0e Compare April 9, 2024 15:58

henrikbrixandersen approved these changes Apr 9, 2024

View reviewed changes

decsny approved these changes Apr 9, 2024

View reviewed changes

henrikbrixandersen requested review from jfischer-no and carlescufi April 9, 2024 19:08

jfischer-no approved these changes Apr 10, 2024

View reviewed changes

carlescufi merged commit 731f8d4 into zephyrproject-rtos:main Apr 10, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

modbus: Respect CONFIG_UART_USE_RUNTIME_CONFIGURE flag #68858

modbus: Respect CONFIG_UART_USE_RUNTIME_CONFIGURE flag #68858

zagor commented Feb 12, 2024

zagor commented Feb 12, 2024 •

edited

Loading

jfischer-no Feb 15, 2024

zagor Feb 15, 2024

jfischer-no Feb 15, 2024

zagor Feb 15, 2024

jfischer-no Feb 15, 2024

zagor Feb 16, 2024

jfischer-no Feb 16, 2024

zagor commented Feb 16, 2024

jfischer-no Feb 16, 2024

zagor commented Feb 21, 2024

zagor commented Mar 7, 2024

jfischer-no left a comment

zagor commented Mar 7, 2024

jfischer-no commented Mar 7, 2024

zagor commented Mar 8, 2024

zagor commented Mar 12, 2024

bjarki-andreasen left a comment

bjarki-andreasen Mar 13, 2024

zagor Mar 13, 2024

bjarki-andreasen Mar 13, 2024

jfischer-no Mar 19, 2024

jfischer-no commented Mar 28, 2024

zagor commented Mar 29, 2024

henrikbrixandersen commented Apr 2, 2024

jfischer-no commented Apr 3, 2024

carlescufi commented Apr 3, 2024

jfischer-no commented Apr 3, 2024

zagor commented Apr 3, 2024

carlescufi commented Apr 9, 2024

		#ifdef CONFIG_UART_USE_RUNTIME_CONFIGURE
		struct uart_config uart_cfg;

modbus: Respect CONFIG_UART_USE_RUNTIME_CONFIGURE flag #68858

modbus: Respect CONFIG_UART_USE_RUNTIME_CONFIGURE flag #68858

Conversation

zagor commented Feb 12, 2024

zagor commented Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zagor commented Feb 16, 2024

Choose a reason for hiding this comment

zagor commented Feb 21, 2024

zagor commented Mar 7, 2024

jfischer-no left a comment

Choose a reason for hiding this comment

zagor commented Mar 7, 2024

jfischer-no commented Mar 7, 2024

zagor commented Mar 8, 2024

zagor commented Mar 12, 2024

bjarki-andreasen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfischer-no commented Mar 28, 2024

zagor commented Mar 29, 2024

henrikbrixandersen commented Apr 2, 2024

jfischer-no commented Apr 3, 2024

carlescufi commented Apr 3, 2024

jfischer-no commented Apr 3, 2024

zagor commented Apr 3, 2024

carlescufi commented Apr 9, 2024

zagor commented Feb 12, 2024 •

edited

Loading