Hi,

Most of you already use the west tool for various things already, including building, flashing, signing and other common operations. One of the main reasons west was introduced was to support building Zephyr with external modules hosted in external repositories. The first module that was split out of the tree was tinycbor and it seems to be working well. It is time now to start moving many of the external components out of the tree and keep the main zephyr code base as lean and as self-contained as possible and we have a few pull requests already lined up. The ultimate goal is to remove all HALs and external libraries to repositories maintained under the zephyr project org on github and yet maintain the main zephyr tree in a state that allows building a representative amount of configuration for testing purposes.

With the changes coming over the next few weeks you need to remember to update the environment with `west update` and pull the latest changes per the manifest from all configured modules. Failing to do so will result in inconsistent build results. So instead of just pulling zephyr (with zephyr pull or zephyr fetch) please make sure you use `west update` to get the latest state of the code in modules whenever the manifest changes.

For more information about module and how to submit changes to modules or how to submit a new module, visit:

https://docs.zephyrproject.org/latest/guides/modules.html

First batch of changes to move ext/ components into modules will start going into the tree starting tomorrow (Wed.)

Thanks,

Anas

On May 21, 2019, at 2:22 PM, Nicolas Pitre <npitre@...> wrote:

Hello,

This is my first post here, however it probably won't be the last.

For a while I've been working on making Zephyr to compile for and run on
64-bit targets. That currently works with the native-posix target,
and 64-bit RISC-V is my next target.

I would like to submit my work for inclusion into the mainline Zephyr
repository. The documented submit and review procedures imply Github
forks and pull requests which involve going through hoops such as a
Javascript-capable web browser and that is not exactly like a walk in
the park for me (my background and credentials are available online if
you want to know why). That would make my life easier if there could
be some alternative way similar to how it is done with the Linux kernel.
Please let me know if that is possible.

I'm therefore posting my first patch here mainly to initiate a
conversation on this issue. Obviously I have many more of them.

----- >8
Subject: [PATCH] fix PTP clock test case

The various ptp_clock_driver_api methods receive a struct ptp_context
not a struct eth_context, and the later must be obtained from a reference
stored in the former. Failing to do so by directly interpreting
driver_data as a struct eth_context instance does end up corrupting
memory past the actual struct ptp_context storage. The test may still
pass as ptp_clock_get() reads from the same out-of-bounds memory as the
previous ptp_clock_set() and therefore the content matches, and with luck
the corrupted memory is not essential to the completion of the test.
But that wasn't my case which allowed me to find this issue.

Fix this by properly obtaining a reference to the struct eth_context
instance via the actual struct ptp_context, and adding one validation
test in my_ptp_clock_set() like the one found in eth_tx() to make sure
we got the right memory.

Signed-off-by: Nicolas Pitre <npitre@...>

diff --git a/tests/net/ptp/clock/src/main.c b/tests/net/ptp/clock/src/main.c
index 1625f6ec18..5d69164694 100644
--- a/tests/net/ptp/clock/src/main.c
+++ b/tests/net/ptp/clock/src/main.c
@@ -175,29 +175,40 @@ static u64_t timestamp_to_nsec(struct net_ptp_time *ts)
return (ts->second * NSEC_PER_SEC) + ts->nanosecond;
}

+struct ptp_context {
+ struct eth_context *eth_context;
+};
+
static int my_ptp_clock_set(struct device *dev, struct net_ptp_time *tm)
{
- struct eth_context *ctx = dev->driver_data;
+ struct ptp_context *ptp_ctx = dev->driver_data;
+ struct eth_context *eth_ctx = ptp_ctx->eth_context;

- memcpy(&ctx->time, tm, sizeof(struct net_ptp_time));
+ if (&eth_context_1 != eth_ctx && &eth_context_2 != eth_ctx) {
+ zassert_true(false, "Context pointers do not match\n");
+ }
+
+ memcpy(&eth_ctx->time, tm, sizeof(struct net_ptp_time));

return 0;
}

static int my_ptp_clock_get(struct device *dev, struct net_ptp_time *tm)
{
- struct eth_context *ctx = dev->driver_data;
+ struct ptp_context *ptp_ctx = dev->driver_data;
+ struct eth_context *eth_ctx = ptp_ctx->eth_context;

- memcpy(tm, &ctx->time, sizeof(struct net_ptp_time));
+ memcpy(tm, &eth_ctx->time, sizeof(struct net_ptp_time));

return 0;
}

static int my_ptp_clock_adjust(struct device *dev, int increment)
{
- struct eth_context *ctx = dev->driver_data;
+ struct ptp_context *ptp_ctx = dev->driver_data;
+ struct eth_context *eth_ctx = ptp_ctx->eth_context;

- ctx->time.nanosecond += increment;
+ eth_ctx->time.nanosecond += increment;

return 0;
}
@@ -207,10 +218,6 @@ static int my_ptp_clock_rate_adjust(struct device *dev, float ratio)
return 0;
}

-struct ptp_context {
- struct eth_context *eth_context;
-};
-
static struct ptp_context ptp_test_1_context;
static struct ptp_context ptp_test_2_context;

On 20 May 2019, at 00:20, pawel.dunaj@... wrote:

I think %s capability should be dropped from the logger’s format string processing and a separate API added specifically to log a C string, which makes an immediate copy.

Here I totally agree. I think that formatting of strings is such a well known standard that nobody will read the fine print for a logger to see what magic may lie under its hood. I think we should either assert that %s is never used and use some other format instead (i.e. force people to check the doc at least once when they hit an error) or keep %s for in place rendering and add new format for people who want to go faster.
Another thing is that assert is disabled by default so many people may never notice the problem (especially for error paths).

On 20 May 2019, at 00:20, pawel.dunaj@... wrote:

I think %s capability should be dropped from the logger’s format string processing and a separate API added specifically to log a C string, which makes an immediate copy.

Here I totally agree. I think that formatting of strings is such a well known standard that nobody will read the fine print for a logger to see what magic may lie under its hood. I think we should either assert that %s is never used and use some other format instead (i.e. force people to check the doc at least once when they hit an error) or keep %s for in place rendering and add new format for people who want to go faster.
Another thing is that assert is disabled by default so many people may never notice the problem (especially for error paths).

Hi,
To optimize time of log processing message construction is deferred. For this reason you cannot use arrays that will not live long enough for logger thread to see them.
In other words you can always use static arrays (like you do when calling `LOG_xyz("%s", "testme")`) but you must never pass anything from the stack (like in your example). You can enable `CONFIG_LOG_IMMEDIATE` but this will affect logging performance and does not actually tells you why the problem happened in the first place.

If you need to print array declared on stack you must call ` log_strdup()` to duplicate it.

Personally I am against this API and I indicated it will cause bugs as people do expect `%s` to simply output a string and they also don't read documentation for something that should just work.
But at least it makes logger faster if you look from the bright side.

Thanks,