Hello Yannis,

On Thu, 13 Jul 2017 14:20:30 +0300
Yannis Damigos <giannis.damigos@...> wrote:

I spotted a following issue: if I gave negative ("change requested")
review (or just the same, positive review), I don't seem to be able
to change it back to "no review".

[]

https://help.github.com/articles/dismissing-a-pull-request-review
mentions "repository administrators or people with write access can
dismiss a review"

Thanks for confirming this. I by now also submitted a Github report,
it borders on a bug that a review author can't dismiss their own review.

Yannis

--
Best Regards,
Paul

Linaro.org | Open source software for ARM SoCs
Follow Linaro: http://www.facebook.com/pages/Linaro
http://twitter.com/#!/linaroorg - http://www.linaro.org/linaro-blog

On Thu, Jul 13, 2017 at 2:12 PM, Paul Sokolovsky
<paul.sokolovsky@...> wrote:

Hello,

I spotted a following issue: if I gave negative ("change requested")
review (or just the same, positive review), I don't seem to be able to
change it back to "no review".

For example, if I looked at some patch and spotted a glaring typo, I'd
feel that warrants "requesting a change". But once submitter fixes it,
I can't reset my review to the neutral state. The only choice is to
approve the PR, but often, that's not a choice because I didn't review
PR in full, or not familiar enough with some subsystem.

https://github.com/blog/2265-dismissing-reviews-on-pull-requests
tells that review dismissal is possible, but doesn't document who can
dismiss a review. I definitely don't see "Dismiss review" links like
shown there, and can't dismiss my own review (which is of course a bug
in Github).

Can someone give suggestions what to do in this case? In particular,
can people with merge permissions check if they're shown "Dismiss
review" links like above?


Thanks,
Paul

Linaro.org | Open source software for ARM SoCs
Follow Linaro: http://www.facebook.com/pages/Linaro
http://twitter.com/#!/linaroorg - http://www.linaro.org/linaro-blog
_______________________________________________
Zephyr-devel mailing list
Zephyr-devel@...
https://lists.zephyrproject.org/mailman/listinfo/zephyr-devel

Hello,

https://help.github.com/articles/dismissing-a-pull-request-review
mentions "repository administrators or people with write access can
dismiss a review"

Yannis

Hello,

I spotted a following issue: if I gave negative ("change requested")
review (or just the same, positive review), I don't seem to be able to
change it back to "no review".

For example, if I looked at some patch and spotted a glaring typo, I'd
feel that warrants "requesting a change". But once submitter fixes it,
I can't reset my review to the neutral state. The only choice is to
approve the PR, but often, that's not a choice because I didn't review
PR in full, or not familiar enough with some subsystem.

https://github.com/blog/2265-dismissing-reviews-on-pull-requests
tells that review dismissal is possible, but doesn't document who can
dismiss a review. I definitely don't see "Dismiss review" links like
shown there, and can't dismiss my own review (which is of course a bug
in Github).

Can someone give suggestions what to do in this case? In particular,
can people with merge permissions check if they're shown "Dismiss
review" links like above?


Thanks,
Paul

Linaro.org | Open source software for ARM SoCs
Follow Linaro: http://www.facebook.com/pages/Linaro
http://twitter.com/#!/linaroorg - http://www.linaro.org/linaro-blog

Hello,

On Mon, 10 Jul 2017 20:47:47 +0000
"Boie, Andrew P" <andrew.p.boie@...> wrote:

We enabled the MMU with an identity page table for the qemu_x86 and
qemu_x86_iamcu board targets. Branching off to bad memory addresses,
dereferencing NULL pointers, etc should now trigger page faults
indicating exactly where the offending instruction occurred.

As a result, the footprint sizes for all the sanity tests on
qemu_x86/qemu_x86_iamcu have greatly increased due to the overhead of
page tables and section alignment. This is expected, will only affect
the QEMU targets, and we've significantly increased the RAM size
available to the emulator so that this should not cause any problems.
Sanitycheck will be reporting these increased footprint sizes as
warnings however.

To follow up, this indeed highlighted various issues in the code,
e.g. with networking subsystem, for example
https://github.com/zephyrproject-rtos/zephyr/issues/752 .

So, all parties who depend on qemu_x86 for their daily testing are
encouraged to try the samples they usually use and ensure they work or
report and investigate issues. (And yep, we need to increase
sanitycheck test coverage to catch these automatically, except that
it's not easy for all subsystems without massive investing into
additional test infrastructure, e.g.
https://jira.zephyrproject.org/browse/ZEP-2118)

Andrew

--
Best Regards,
Paul

Linaro.org | Open source software for ARM SoCs
Follow Linaro: http://www.facebook.com/pages/Linaro
http://twitter.com/#!/linaroorg - http://www.linaro.org/linaro-blog

Hi all,

I have prepared a proposal for the I2S (Inter-IC Sound) driver API.

https://github.com/zephyrproject-rtos/zephyr/pull/745

The list of the most important design decision I've made follows:

I2S (Inter-IC Sound) serial interface can carry relatively large amount
of continuous, uninterrupted data. To support this requirement the
design of this API is built around TX and RX queues which are filled /
emptied by the user application. In a typical case an application will
pre-fill the TX queue with some data and start the transmission via a
trigger command. This is a similar design to those used by Linux and
other embedded OSes.

To prevent unnecessary copying of data buffers between an application
and the I2S driver the TX/RX queues use Zephyr memory slabs. A TX memory
block containing data to transmit will be reserved from the pool by the
application and released by the I2S driver after all data are sent,
similarly a RX memory block will be reserved by the I2S driver and
released by the application. The memory slabs are initialized by and
belong to the application to play well with MPU region protection.

Supported features:

The API allows to configure TX/RX stream format independently. Apart
from the I2S standard common non-standard extensions such as PCM
Short/Long Frame Sync, Left/Right Justified Data Format are also
supported. The transmission/reception can start/end independently or
synchronously. In case of transmit/reception error the respective queue
is blocked to allow application to re-synchronize the data flow.

Both blocking and non-blocking calls are supported.

The different possible scenarios of transmit/receive clock, frame sync
and data signal connections between master and slave devices are
supposed to be handled by the DTS as signal routing never changes at run
time.

The API is not complete and will have to be extended in the future, e.g.
we need more elaborate handling of TDM data. Nevertheless a large class
of I2S applications is already supported.

This PR includes an example I2S application at
samples/drivers/i2s/src/main.c

Any comments are welcome.

Cheers,
Piotr

On Tue, Jul 11, 2017 at 01:05:56PM +0000, Puzdrowski, Andrzej wrote:

I think it's more suitable to get pages size as it is quantum of the
erase. Instead of get map in run-length format I propose to query by
address like here:
https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_NORDIC/TARGET_NRF5/flash_api.c#L93
It will be very easy possible to produce such a map using similar API
- but usually map is easy to describe in a "get" algorithm.

This does make sense. Having the get API makes the code very simple
for the majority of devices where the sector sizes are uniform in
size. The only thing is that if code needs to know if the sectors are
uniform in size, it would have to scan through the device to learn
this. I can see this as a common optimization, or in the case of a
filesystem, even a requirement.

For mcuboot, the address query would be fine.

David

Hi David. Thanks for your answer.

Bellow I responds to your comments:

However, at least all of the drivers that I've used will write larger blocks than the write unit of the device. An unlock, a large write, and a lock seem to work. It is possible that
there is a driver that implements the undersirable but apparently allowed behavior of locking after an individual write.
I think the solution here should be to fix the wording of the API, since it makes the driver almost useless for normal operation.

I totally agree.

- Writes must happen in units of 1-8 bytes, depending on the device.
Most of the devices with larger write units do not allow rewrites
of the same area, even if it is just to clear bits (it either
wears the device excessively, or in one case, there is an internal
ECC that can't be rewritten).

Agree on that - driver shouldn’t break hardware specification If hw doesn't allow rewrites then driver shouldn't try to allow it.
For some devices it is possible to write to the same word again (like for Nordic). I experienced a few project where it was very useful to use the sub-granuality , the sub-alignment and rewrite provided by the memory driver. I propose to provide interface to get booth physical alignment and those provided by the driver. I think both are useful - hw aligment would be useful for future wear leverage algorithm (e.g incorporated into a file system)

Sector layout query. Most flash devices have some number of
uniform sectors. There are some, that have a generally smaller
number of larger sectors (often of differing size). Representing
the sector map in a run-length format should cover both of these
cases.

I think it's more suitable to get pages size as it is quantum of the erase. Instead of get map in run-length format I propose to query by address like here: https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_NORDIC/TARGET_NRF5/flash_api.c#L93
It will be very easy possible to produce such a map using similar API - but usually map is easy to describe in a "get" algorithm.

I plan to provide PR for discussion soon.

Andrzej

-----Original Message-----
From: David Brown [mailto:david.brown@...]
Sent: Monday, July 10, 2017 4:54 PM
To: Puzdrowski, Andrzej <Andrzej.Puzdrowski@...>
Cc: zephyr-devel@...
Subject: Re: [Zephyr-devel] Flash drivers compatibility attitude

On Fri, Jul 07, 2017 at 10:01:12AM +0000, Puzdrowski, Andrzej wrote:

It is unwanted freedom because it is the driver API and hence it should
provide abstraction over HW e.g write operation could perform a write
of a long buffer despite that HW write is word-by-word operation so HW
protect must be served by a such driver automatically and protection
offered by this API should be the abstaraction over HW.

There does have to be some knowledge of what the flash device is doing in order for an application to be able to use the flash robustly.

However, at least all of the drivers that I've used will write larger blocks than the write unit of the device. An unlock, a large write, and a lock seem to work. It is possible that there is a driver that implements the undersirable but apparently allowed behavior of locking after an individual write.

I think the solution here should be to fix the wording of the API, since it makes the driver almost useless for normal operation.

It is important that this API be fairly directly mapped onto the device. I think looping over a write is OK because it doesn't cause any unexpected behavior if power is lost during the operation.

However, a driver that automatically erases and rewrites when it detects a write smaller than the hardware allows should not be allowed. If power is lost, it can lost significantly more data than just the data being written. If we find this kind of functionality in existing drivers, I would argue that it should be removed.

So far, the common capabilities I've found in the NOR flashes used in Zephyr MCUs are:

- Erase sectors are fairly large (2k - 128k) and may be uniform, or
vary in size across the device.

- Writes must happen in units of 1-8 bytes, depending on the device.
Most of the devices with larger write units do not allow rewrites
of the same area, even if it is just to clear bits (it either
wears the device excessively, or in one case, there is an internal
ECC that can't be rewritten).

2)      Alignment and write granularity of F. flash_write:

there’s no dox regard how unaligned data should be treated. For
instance the driver for nrf5x support 1 byte alignment and data size
form 1B to N B. Other driver expected different alignment here (for
instance qmsi driver expect double-word alignment). Solution for this
is to describe this requirement beater or make the common requirement
for all drivers. In the case when alignment requirements will be kept
different for each driver I would consider the addition to the driver
API to get the requirements.

Agreed here. With mcuboot, we have to have local knowledge of what the driver's write alignment is.

As we've been adding device tree support, we've already defined a way of representing flash partitions in the device tree.

I think the appropriate calls to add to the flash API are:

- Sector layout query. Most flash devices have some number of
uniform sectors. There are some, that have a generally smaller
number of larger sectors (often of differing size). Representing
the sector map in a run-length format should cover both of these
cases.

- Write alignment query. This is just a small integer, but it is
important to be able to write correctly.

- Device name. The name of the driver to use is somewhat ad-hoc,
and can be set in the config. In mcuboot's target-specific
headers, we usually having something along the lines of:

#define FLASH_DRIVER_NAME CONFIG_SOC_FLASH_MCUX_DEV_NAME

in order to get the name. But the config used is different for
each device.

I was planning on adding these queries to the API at some point in order to be able to move this device-specific information out of mcuboot.

David

On Fri, Jul 07, 2017 at 10:01:12AM +0000, Puzdrowski, Andrzej wrote:

It is unwanted freedom because it is the driver API and hence it
should provide abstraction over HW e.g write operation could perform a
write of a long buffer despite that HW write is word-by-word operation
so HW protect must be served by a such driver automatically and
protection offered by this API should be the abstaraction over HW.

There does have to be some knowledge of what the flash device is doing
in order for an application to be able to use the flash robustly.

However, at least all of the drivers that I've used will write larger
blocks than the write unit of the device. An unlock, a large write,
and a lock seem to work. It is possible that there is a driver that
implements the undersirable but apparently allowed behavior of locking
after an individual write.

I think the solution here should be to fix the wording of the API,
since it makes the driver almost useless for normal operation.

It is important that this API be fairly directly mapped onto the
device. I think looping over a write is OK because it doesn't cause
any unexpected behavior if power is lost during the operation.

However, a driver that automatically erases and rewrites when it
detects a write smaller than the hardware allows should not be
allowed. If power is lost, it can lost significantly more data than
just the data being written. If we find this kind of functionality in
existing drivers, I would argue that it should be removed.

So far, the common capabilities I've found in the NOR flashes used in
Zephyr MCUs are:

- Erase sectors are fairly large (2k - 128k) and may be uniform, or
vary in size across the device.

- Writes must happen in units of 1-8 bytes, depending on the device.
Most of the devices with larger write units do not allow rewrites
of the same area, even if it is just to clear bits (it either
wears the device excessively, or in one case, there is an internal
ECC that can't be rewritten).

2)      Alignment and write granularity of F. flash_write:

there’s no dox regard how unaligned data should be treated. For
instance the driver for nrf5x support 1 byte alignment and data size
form 1B to N B. Other driver expected different alignment here (for
instance qmsi driver expect double-word alignment). Solution for this
is to describe this requirement beater or make the common requirement
for all drivers. In the case when alignment requirements will be kept
different for each driver I would consider the addition to the driver
API to get the requirements.

Agreed here. With mcuboot, we have to have local knowledge of what
the driver's write alignment is.

As we've been adding device tree support, we've already defined a way
of representing flash partitions in the device tree.

I think the appropriate calls to add to the flash API are:

- Sector layout query. Most flash devices have some number of
uniform sectors. There are some, that have a generally smaller
number of larger sectors (often of differing size). Representing
the sector map in a run-length format should cover both of these
cases.

- Write alignment query. This is just a small integer, but it is
important to be able to write correctly.

- Device name. The name of the driver to use is somewhat ad-hoc,
and can be set in the config. In mcuboot's target-specific
headers, we usually having something along the lines of:

#define FLASH_DRIVER_NAME CONFIG_SOC_FLASH_MCUX_DEV_NAME

in order to get the name. But the config used is different for
each device.

I was planning on adding these queries to the API at some point in
order to be able to move this device-specific information out of
mcuboot.

David

Hi,

Since it seems this issue is flying under the radar lets have it first
discussed here since there might be implemented for the general design
of many kernel objects:

https://github.com/zephyrproject-rtos/zephyr/pull/682

So many of the insert/push APIs do end up doing the following:

if (first_pending_thread) {
prepare_thread_to_run(first_pending_thread, data);
if (!_is_in_isr() && _must_switch_threads()) {
(void)_Swap(key);
return;
}
} else {
sys_slist_insert(&queue->data_q, prev, data);
if (handle_poll_event(queue)) {
(void)_Swap(key);
return;
}
}

So the first part just basically bypass the list entirely and post the
data directly into thread's swap_data, thus making it impossible to
cancel. There is however a way to fix this in case of k_work, but it
does involve using k_poll instead of k_fifo_get, as in that case the
list insert is used we can, in fact, remove the items, though the
workqueue thread still wakes up but using k_fifo_get would return NULL
which we could basically ignore as it seems to be an acceptable
behavior since it is the current way to wakeup waiters.

If we do decide to use k_pool in k_work we will obviously need to
rework the kconfig to add a dependency, so either k_work is no longer
built-in by default and we do have a config option for it, e.g:
CONFIG_WORK, or we remove CONFIG_POLL and make it always available.
Both ways there will be an increase in the memory requirements.

In case we do want k_poll to be built-in by default we may as well
redesign this code around posting data directly to thread's swap data,
instead, everything would be done via k_poll then we only need
handle_poll_event to do its magic.

--
Luiz Augusto von Dentz

Hello All,

I was looking into the STM32 CRYPT, HASH and CRC32 units and how they
fit in Zephyr and could not really figure it out.

There is a drivers/crypto/ with some "real" drivers for doing encryption
and decryption of data via a Zephyr "device driver".

There is also a drivers/crc/ that holds only a function to do crc16
without any "device driver".

And there doesn't seem to be anything that has to do with hashes like
SHA1, MD5 etc. (except for the CRC which is technically also a hash).

So now my problem is where and how the 3 STM32 devices CRYPT, HASH and
CRC32 could be added to Zephyr in a way that they are also generally
useful.

For example how to deal with the limitations of hardware like the
STM32F4 CRC32 unit (it only supports one fixed polynomial and 0xffffffff
as start value)?

- Erwin

Hello Andrzej,

On Fri, 7 Jul 2017 10:01:12 +0000
"Puzdrowski, Andrzej" <Andrzej.Puzdrowski@...> wrote:

In my opinion Flash driver API hasn't well-defined common behavior.
It could cause additional complexity in component added on top of
flash driver because the incompatibility must be deferred at higher
component level. I have 2 points:

I can't say much about Flash API, but the above paragraph reminds me
well about my own thoughts/wording about following problems:

1. Device naming inconsistencies, in particular, GPIO naming, e.g. one
port uses "GPIO_1", some another - "GPIOA".
2. Similar consistency/well-documentedness of UART API. This was
tracked as https://jira.zephyrproject.org/browse/ZEP-2016 and I made
some effort to alleviate it in 1.8.0 release.

So, without having much experience with Flash API, or even looking into
it, I can give an "ack" that, with Zephyr being fast-growing and not
fully mature, there can be API issues like you mention, and would like
to encourage you to drive this matter forward - we need more people
paying attention to such issues ;-).

[]

Andrzej Puzdrowski

--
Best Regards,
Paul

Linaro.org | Open source software for ARM SoCs
Follow Linaro: http://www.facebook.com/pages/Linaro
http://twitter.com/#!/linaroorg - http://www.linaro.org/linaro-blog

On 05/07/17 19:19, Marcio Montenegro wrote:

http://www.eejournal.com/article/intel-kills-off-embedded-boards/

Don't you think this article is just a both overstated?

Companies do discontinue boards from time to time. It's normal. There remain plenty of zephyr-supported boards to choose from:

https://www.zephyrproject.org/doc/boards/boards.html


Daniel.