Skip to content

Added file backend support to flash simulator for posix architecture #20553

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Dec 9, 2019
Merged

Added file backend support to flash simulator for posix architecture #20553

merged 2 commits into from
Dec 9, 2019

Conversation

vanwinkeljan
Copy link
Member

Extended flash simulator for posix architecture to support writing/reading from a binary file.

Further removed the native_posix flash driver as its functionality is merged with the flash simulator driver.

@zephyrbot zephyrbot added area: Boards area: native port Host native arch port (native_sim) area: Tests Issues related to a particular existing or missing test labels Nov 10, 2019
@zephyrbot
Copy link
Collaborator

zephyrbot commented Nov 10, 2019 •
edited
Loading

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@Laczen
Copy link
Collaborator

Laczen commented Nov 10, 2019

Hi @vanwinkeljan, this is a great proposal. Do you think the addition of a lock for thread safety is required?

@Laczen Laczen mentioned this pull request Nov 10, 2019
Merged
@vanwinkeljan
Copy link
Member Author

Do you think the addition of a lock for thread safety is required?

@Laczen do you mean in native posix related code or the simulated flash driver in general?

In the first case (native_posix specific code) no additional locking is needed as by design in native posix there is only one thread is active at the time.

In the second case locking is need (and in place) as concurrent access could happen to the flash device.

@Laczen
Copy link
Collaborator

Laczen commented Nov 10, 2019 •
edited
Loading

Do you think the addition of a lock for thread safety is required?

@Laczen do you mean in native posix related code or the simulated flash driver in general?

In the first case (native_posix specific code) no additional locking is needed as by design in native posix there is only one thread is active at the time.

In the second case locking is need (and in place) as concurrent access could happen to the flash device

I mean for the simulated flash driver, I think it is missing. In the simulated eeprom I am only adding it when multithread is selected. BTW do you think it is needed to support multiple simulated flash/eeprom devices ?

@vanwinkeljan
Copy link
Member Author

I mean for the simulated flash driver, I think it is missing. In the simulated eeprom I am only adding it when multithread is selected

Looks like I mixed up both (flash/eeprom) drivers when looking at the code and the flash driver is missing any form of locking.

BTW do you think it is needed to support multiple simulated flash/eeprom devices ?

On the long run I would say yes, but we first we need a generic (generated) way of supporting multiple instance of a driver

@vanwinkeljan vanwinkeljan force-pushed the merge_native_sim_flash_driver branch from 065cada to c631a4d Compare November 11, 2019 09:28
aescolar
aescolar requested changes Nov 11, 2019
View reviewed changes
Copy link
Member

@aescolar aescolar left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing serious found by my review, but CI did :)

dbkinder
dbkinder suggested changes Nov 11, 2019
View reviewed changes
boards/posix/native_posix/doc/index.rst Outdated
Further the emulated device will not impose any write restriction that are
applicable for a regular flash device, including changing the state of a bit
from zero to one.
on the host file system. The behaviour of the flash device can be configured
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

our project preference is for American English, so change to behavior

@vanwinkeljan
drivers/flash/flash_simulator: Added file back end for posix arch
fbd57c7 
Extended flash simulator for posix architecture to read/write data
from a binary file on the host file system.

Further enable the flash simulator by default on native_posix(_64)
boards and updated the documentation accordingly.

Signed-off-by: Jan Van Winkel <[email protected]>
@vanwinkeljan
drivers/flash: Removed native posix flash driver
d3f9095 
Removed native posix flash driver as functionality is merged with flash
simulator driver.

Signed-off-by: Jan Van Winkel <[email protected]>
@vanwinkeljan vanwinkeljan force-pushed the merge_native_sim_flash_driver branch from c631a4d to d3f9095 Compare November 12, 2019 19:26
@Laczen
Copy link
Collaborator

Laczen commented Nov 12, 2019

@vanwinkeljan, no lock for multithreading yet?

@vanwinkeljan
Copy link
Member Author

@vanwinkeljan, no lock for multithreading yet?

No, rather do that in a separate PR as this one is posix arch specific and no locking is needed in that case.

@Laczen
Copy link
Collaborator

Laczen commented Nov 12, 2019

@vanwinkeljan, no lock for multithreading yet?

No, rather do that in a separate PR as this one is posix arch specific and no locking is needed in that case.

Ok, I will approve.

@nvlsianpu nvlsianpu requested a review from kapi-no November 13, 2019 10:34
nvlsianpu
nvlsianpu previously requested changes Nov 13, 2019
View reviewed changes
Copy link
Collaborator

@nvlsianpu nvlsianpu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pleas await for @kapi-no review

@aescolar
Copy link
Member

aescolar commented Nov 13, 2019 •
edited
Loading

@nvlsianpu Note that we are in freeze, so this won't make it in anyhow for the next 2.5 weeks.

@nvlsianpu
Copy link
Collaborator

@aescolar We are able to be extremely slow responsive :D

@vanwinkeljan
Copy link
Member Author

@kapi-no any input?

kapi-no
kapi-no approved these changes Nov 22, 2019
View reviewed changes
Copy link
Collaborator

@kapi-no kapi-no left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of having a common flash simulator for all simulator platforms. Thank you for your effort and sorry for the late review.

@vanwinkeljan vanwinkeljan dismissed nvlsianpu’s stale review December 7, 2019 10:28

kapi-no reviewed PR

@nashif nashif merged commit cba129a into zephyrproject-rtos:master Dec 9, 2019
@vanwinkeljan vanwinkeljan deleted the merge_native_sim_flash_driver branch December 10, 2019 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nashif nashif nashif approved these changes

@Laczen Laczen Laczen approved these changes

@henrikbrixandersen henrikbrixandersen henrikbrixandersen approved these changes

@dbkinder dbkinder dbkinder approved these changes

@kapi-no kapi-no kapi-no approved these changes

@aescolar aescolar aescolar approved these changes

@pabigot pabigot Awaiting requested review from pabigot

@wentongwu wentongwu Awaiting requested review from wentongwu

@nvlsianpu nvlsianpu Awaiting requested review from nvlsianpu

Assignees
No one assigned
Labels
area: Boards area: Flash area: native port Host native arch port (native_sim) area: Tests Issues related to a particular existing or missing test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@vanwinkeljan @zephyrbot @Laczen @aescolar @nvlsianpu @nashif @henrikbrixandersen @dbkinder @kapi-no