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.

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

Add NVStore (A.K.A SOTP) feature #5900

Merged
merged 5 commits into from Mar 2, 2018

Conversation

@davidsaada
Contributor

davidsaada commented Jan 23, 2018

Description

Adding the NVStore feature.

Status

READY

Migrations

If this PR changes any APIs or behaviors, give a short description of what API users should do when this PR is merged.

NO

Related PRs

None.

branch PR
other_pr_production link
other_pr_master link

Todos

  • Documentation
  • Need to move configuration of area addresses and sizes from targets.json to local mbed_lib.json.

Steps to test or reproduce

See README.md for testing instructions.

@cmonr cmonr added the needs: review label Jan 23, 2018


#include "nvstore_shared_lock.h"

#include "mbed.h"

This comment has been minimized.

@0xc0170

0xc0170 Jan 23, 2018

Member

replace this header file with inclusion what is required here in this file

@@ -0,0 +1,120 @@
/*
* Copyright (c) 2016 ARM Limited. All rights reserved.

This comment has been minimized.

@0xc0170

0xc0170 Jan 23, 2018

Member

2018 should be year for all new files

NVSTORE_OS_NO_MEM_ERR = -3,
};

class SharedLock {

This comment has been minimized.

@0xc0170

0xc0170 Jan 23, 2018

Member

I am interested in this SharedLock - missing docs here to describe the scope of this. And if we do not have already something similar in the codebase?

This comment has been minimized.

@davidsaada

davidsaada Jan 23, 2018

Contributor

This definitely needs documentation, which will be added. This class implements a shared lock mechanism (AKA RW lock). Now this implementation isn't like the classic one ,the one that needs two Mutexes and a counter. Instead it has one Mutex and an atomic counter, favoring the usage of shared lock/unlock on the count of the exclusive lock/unlock. This will be added in the documentation.

This comment has been minimized.

@davidsaada

davidsaada Jan 23, 2018

Contributor

And there doesn't seem to be an implementation for it, at least not under rtos.


size_t nvstore_int_flash_get_sector_size(uint32_t address);

int nvstore_int_flash_init(void);

This comment has been minimized.

@0xc0170

0xc0170 Jan 23, 2018

Member

should this API be documented in this header file?

This comment has been minimized.

@davidsaada

davidsaada Jan 23, 2018

Contributor

Will add short documentation. This is basically a wrapper for the FlashIAP operations, but it should be documented indeed.

}

while (size)
{

This comment has been minimized.

@0xc0170

0xc0170 Jan 23, 2018

Member

on the line 172

This comment has been minimized.

@davidsaada

davidsaada Jan 23, 2018

Contributor

Please elaborate. Seems like a partial comment.

This comment has been minimized.

@0xc0170

0xc0170 Jan 23, 2018

Member

{ misplaced (as the rest of file has it fine)

This comment has been minimized.

@davidsaada

davidsaada Jan 23, 2018

Contributor

OK thanks.


// ----------------------------------------------------------- Includes -----------------------------------------------------------

#include "mbed.h"

This comment has been minimized.

@0xc0170

0xc0170 Jan 23, 2018

Member

same as another comment, include what is needed here

you could do - first include project header files, then libs.

// Use this implementation of singleton (Meyer's) rather than the one that allocates
// the instance on the heap, as it ensures destruction at program end (preventing warnings
// from memory checking tools such as valgrind).
static NVStore instance;

This comment has been minimized.

@0xc0170

0xc0170 Jan 23, 2018

Member

thread safe for the C++ standard we use at the moment?

This comment has been minimized.

@davidsaada

davidsaada Jan 23, 2018

Contributor

Yes - this implementation is thread safe. It's similar to the static implementation tested in TESTS/mbed_drivers/race_test.

This comment has been minimized.

@pan-

pan- Jan 24, 2018

Member

Meyer's singleton is thread safe in C++11 and later revisions of the language; not C++98/03.
Note that it's supposed to be safe with ARM ABI with the help of __cxa_guard_acquire.

This comment has been minimized.

@davidsaada

davidsaada Jan 24, 2018

Contributor

Indeed so. However, as far as I managed to understand, this specific feature (accessing static member for the first time by multiple threads) has been verified to work in our currently supported tool chains. I don't mind adding a lock there, but this would affect performance (each time get_instance is invoked).
@c1728p9, @geky - care to elaborate about this?

This comment has been minimized.

@davidsaada

davidsaada Jan 24, 2018

Contributor

Anyway, I can add a lock there the same way as SingletonPtr does.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 25, 2018

Contributor

We have quite a bit of code relying on this being thread-safe, and I believe/hope it is with all our toolchains.

Technically nothing's thread-safe in C++98/03, because they don't have threads.

This comment has been minimized.

@0xc0170

0xc0170 Jan 25, 2018

Member

We provide guards for ARMCC and GCC, not certain about IAR but should (they mention guard calls and multithreaded lib support that we both enabled).

should be fine as it is

This comment has been minimized.

@pan-

pan- Jan 25, 2018

Member

@kjbracey-arm Out of curiosity I've made a quick check. It is safe with all the compilers we support.

This comment has been minimized.

@geky

geky Jan 25, 2018

Member

As @davidsaada mentioned, there's also a test specificaly to check this in mbed-os:
https://github.com/ARMmbed/mbed-os/blob/master/TESTS/mbed_drivers/race_test/main.cpp#L50

This comment has been minimized.

@davidsaada

davidsaada Jan 29, 2018

Contributor

So right now I'm keeping this as is.

// wait until init is finished.
_init_attempts_val = safe_increment(&_init_attempts, 1);
if (_init_attempts_val != 1) {
#if NVSTORE_THREAD_SAFETY

This comment has been minimized.

@0xc0170

0xc0170 Jan 23, 2018

Member

NVSTORE_THREAD_SAFE vs NVSTORE_THREAD_SAFETY

This comment has been minimized.

@davidsaada

davidsaada Jan 23, 2018

Contributor

See comment below regarding thread safety.

#include <stdlib.h>
#include <stdio.h>

#ifndef NVSTORE_THREAD_SAFE

This comment has been minimized.

@0xc0170

0xc0170 Jan 23, 2018

Member

Why is this macro needed? Is it explained anywhere? Why would we want to disable this?

This comment has been minimized.

@davidsaada

davidsaada Jan 23, 2018

Contributor

Originally this was there in the client code in order to allow users to reduce code size, if it is clear that no thread safety is required (for instance if there's only one thread). This will be removed altogether, as well as the NVSTORE_PROBE_ONLY define (reducing code size even more aggressively).

@davidsaada davidsaada force-pushed the davidsaada:david_sotp_cpp branch from 197fb88 to f9da807 Jan 24, 2018

@0xc0170 0xc0170 requested review from sg- and SenRamakri Jan 24, 2018

### Configuring NVStore for your board
NVStore requires the addresses and sizes of both areas in flash. For this purpose, the following values should be defined in
configuration:
NVSTORE_AREA_1_ADDRESS

This comment has been minimized.

@0xc0170

0xc0170 Jan 24, 2018

Member

separate with , or use a list, otherwise this becomes one liner that is not much readable

### Using NVStore
NVStore is a singleton class, meaning that the system can have only a single instance of it.
To instanciate NVStore, one needs to call its get_instance member function as following:
NVStore &nvstore = NVStore::get_instance();

This comment has been minimized.

@0xc0170

0xc0170 Jan 24, 2018

Member

Should use code highlight

code 
@@ -0,0 +1,18 @@
{

This comment has been minimized.

@0xc0170

0xc0170 Jan 24, 2018

Member

is this json needed - its not usable in this codebase

This comment has been minimized.

@davidsaada

davidsaada Jan 29, 2018

Contributor

Removed.

#if defined(NVSTORE_AREA_1_ADDRESS) && defined(NVSTORE_AREA_1_SIZE) && \
defined(NVSTORE_AREA_2_ADDRESS) && defined(NVSTORE_AREA_2_SIZE) && \
defined(DEVICE_FLASH)
#define NVSTORE_ENABLED 1

This comment has been minimized.

@0xc0170

0xc0170 Jan 24, 2018

Member

what is the intention here with enabled flag - why cant this always be present?

What others think about this ?

This comment has been minimized.

@davidsaada

davidsaada Jan 24, 2018

Contributor

This was actually proposed by the folks from the Austin team. Right now, NVStore is currently supported on two boards (K64F & K82F) only. The defines configuring NVStore addresses and sizes must be there for each supported board, otherwise the module will not build properly. However, we don't want to fail the build for all other boards. So, any board that has these defines is considered to support NVStore, with the enabled define denoting it. All other boards will fail the build only when trying to access the NVStore class. In addition, the NVStore test is declared unsupported based on this define (much cleaner than checking all other defines again).

#include "nvstore_int_flash_wrapper.h"
#include "nvstore_shared_lock.h"

enum {

This comment has been minimized.

@0xc0170

0xc0170 Jan 24, 2018

Member

shall this have a name, and return types in the NVStore to be type of this enumeration ??

also limit the scope to be in the class?

This comment has been minimized.

@davidsaada

davidsaada Jan 29, 2018

Contributor

Wanted to keep it as simple as possible, having all functions returning int (as it's done in some of the block devices code). Is there any common practice here (int vs. enum)?

*
* @returns Number of keys.
*/
uint16_t get_max_keys();

This comment has been minimized.

@0xc0170

0xc0170 Jan 24, 2018

Member

can be const, similar other methods like size()

This comment has been minimized.

@davidsaada

davidsaada Jan 29, 2018

Contributor

This one can indeed be const and will be modified. The size function can't, as it lazily calls the init function, required in order to determine the NVStore size.

/**
* @brief Programs one item of data on Flash, given key, allowing no consequent sets to this key.
*
* @param [in] key

This comment has been minimized.

@0xc0170

0xc0170 Jan 24, 2018

Member

can these be on the same line

@param [in] key Key of stored item.
// Declare these (not implemented) to make sure they're not accidentally declared
// by derived classes
NVStore(NVStore const&);
void operator=(NVStore const&);

This comment has been minimized.

@0xc0170

0xc0170 Jan 24, 2018

Member

NonCopyable class in the codebase for this

* Instead, it uses one Mutex and one atomic counter, favouring the shared usage of the exclusive one.
*/

class SharedLock {

This comment has been minimized.

@0xc0170

0xc0170 Jan 24, 2018

Member

@kjbracey-arm Can you review this SharedLock class ?

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 25, 2018

Contributor

This is a SharedMutex, not a SharedLock.

If this was for general use, I'd be happier without it having a "poll and retry", so that a writer can get in immediately. A ConditionVariable would enable proper waking.

And I'd be happier with a fairer algorithm - Terekhov's is the classic - http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2406.html and https://stackoverflow.com/questions/12082405/boost-shared-lock-read-preferred . The condition-variable-based algorithms there would be implementable in mbed OS now.

The promote functionality is a trap - it will deadlock if ever called from two threads. NVStore's extra synchronisation with the atomic free space pointer avoids that, but having it just there in the API without warnings is scary.

Other notes would be to standardise naming - lock/unlock (making #5852 work with it) and lock_shared/unlock_shared and eliminate the return values - "can-fail" locks call too many problems. (I believe there's broad agreement to make non-timed Mutexs assert on failure.)

But I'm not sure we really want to spend much time trying to design "mbed OS" shared locks at this point - we've had enough trouble with the ScopedLock.

I would kind of suggest we just accept this as-is for NVStore's internal use - rename it with some sort of NVStore prefix or namespace to avoid collisions with any potential general-purpose one. Maybe we could start a separate issue about agreeing an API/implementation for general use elsewhere. Don't want to totally derail this PR.

This comment has been minimized.

@davidsaada

davidsaada Jan 29, 2018

Contributor

Thanks for the thorough review. This was indeed implemented privately for NVStore, with the intention of favoring the shared usage (writers) over the exclusive one (GC), which is why I didn't use the classic two mutexes implementation. Guess that if there was such an mbed-os class, I'd use it instead. Anyway I'll add an NVStore namespace there to keep it bounded to NVStore.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 29, 2018

Contributor

Fine. But do check my top-level PR comments that I failed to attach to the diff - I think there may be a couple of flaws in how it's used by nvstore.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 24, 2018

Thanks for the quick update for my comments, I reviewed again, left few comments, nothing major.

I added 2 reviewers, if you know someone else, please mention them here, assign as reviewers.

@0xc0170 0xc0170 added needs: work and removed needs: review labels Jan 24, 2018

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Jan 24, 2018

Sam/Martin - It has just occurred to me that the NVSTORE feature I had added in a few config files (like targets.json) was probably redundant. It probably was required when the feature had been under the FEATURE_NVSTORE directory, but not any more as this directory was renamed to nvstore. Is this correct? If so, I'll remove these occurrences.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 25, 2018

Sam/Martin - It has just occurred to me that the NVSTORE feature I had added in a few config files (like targets.json) was probably redundant. It probably was required when the feature had been under the FEATURE_NVSTORE directory, but not any more as this directory was renamed to nvstore. Is this correct? If so, I'll remove these occurrences.

If not needed, please remove

@kjbracey-arm

Can't seem to actually get the GitHub diff of nvstore.cpp to load in a way I can comment on.

So two notes on the "wait for another thread to GC" bit.

  1. There seems to be an out-of-place exclusive_unlock before the loop. Do you mean shared_unlock?
  2. The 100% CPU spin isn't ideal, and isn't the active area check racy, in theory? Couldn't someone other writes cause GC to happen again and flip back to the same area? For now, how about releasing the shared lock, waiting 1ms (given that the lock does that anyway), then just retrying the whole operation from line 634.
* Instead, it uses one Mutex and one atomic counter, favouring the shared usage of the exclusive one.
*/

class SharedLock {

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 25, 2018

Contributor

This is a SharedMutex, not a SharedLock.

If this was for general use, I'd be happier without it having a "poll and retry", so that a writer can get in immediately. A ConditionVariable would enable proper waking.

And I'd be happier with a fairer algorithm - Terekhov's is the classic - http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2406.html and https://stackoverflow.com/questions/12082405/boost-shared-lock-read-preferred . The condition-variable-based algorithms there would be implementable in mbed OS now.

The promote functionality is a trap - it will deadlock if ever called from two threads. NVStore's extra synchronisation with the atomic free space pointer avoids that, but having it just there in the API without warnings is scary.

Other notes would be to standardise naming - lock/unlock (making #5852 work with it) and lock_shared/unlock_shared and eliminate the return values - "can-fail" locks call too many problems. (I believe there's broad agreement to make non-timed Mutexs assert on failure.)

But I'm not sure we really want to spend much time trying to design "mbed OS" shared locks at this point - we've had enough trouble with the ScopedLock.

I would kind of suggest we just accept this as-is for NVStore's internal use - rename it with some sort of NVStore prefix or namespace to avoid collisions with any potential general-purpose one. Maybe we could start a separate issue about agreeing an API/implementation for general use elsewhere. Don't want to totally derail this PR.

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Jan 29, 2018

@kjbracey-arm - for some reason, can't reply on your two "wait for another thread" comments, so answering you here:

  1. Good catch. Changed to shared_unlock.
  2. I guess you referred to the code in the do_get function here. This shouldn't have been spinning, Forgot the 1 msec wait, will add it. The race condition you described (GC returning to the same area) is very very unlikely to happen (as Flash writes also wait on mutexes), but it's valid and should be addressed. Not sure I understand your comment about releasing the shared lock here. Up until now, readers weren't part of the locking/unlocking game, because it wasn't needed (only problematic case was the reader-GC encounters, to which you referred in this comment). Maybe a shared lock and unlock can wrap the next retries, but I don't think it'll solve the race condition you have described.
@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Jan 29, 2018

The racy active area check I meant was at line 666 - writer finding themselves out of space and waiting for someone else to do GC, and deducing it had happened by seeing active area flip (and then assuming there would now be space). That would be solid if they instead they just released the shared lock, waited 1ms, then tried the whole "try-to-claim-space" process again - (I might do it by a "goto 634", but I'm more of a goto fan than many).

I hadn't spotted that one in do_get. I'll leave that with you.

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Jan 29, 2018

Gotcha. Will review this code again considering this race condition.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Jan 29, 2018

For future enhancement, you could consider ConditionVariable - this is an ideal use case, to replace the 1ms delay and retry loop.

Although I suppose in this case because you're using a SharedMutex, you'd actually need a ConditionVariableAny<SharedMutex>. We currently don't have ConditionVariableAny. You could maybe pop that alongside your private SharedMutex. Implementation from the C++ paper will work - again I have a draft kicking around somewhere.

Looked at do_get a bit. I'm thinking you're trying to maybe be overclever and lock-free here, and I'm really not sure why. This is not performance critical-code, surely?

I would have thought the reader should claim the same shared mutex as writes do. Readers and writers both have the same sort of relationship with GC. (That would also give you automatic wake-up when GC finishes, just using the SharedMutex implementation).

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Jan 29, 2018

Absolutely right. This was in fact what I planned of doing following the problem raised in do_get.

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Jan 29, 2018

Committed all changes based on @0xc0170 and @kjbracey-arm comments.
@0xc0170 - Wasn't sure about your comment regarding enums (see above).
@kjbracey-arm - Code modified reflecting our conversation. Would appreciate a re-review of the modified parts (mainly the do_get and do_set functions).

if (save_active_area != _active_area) {
new_free_space = safe_increment(_free_space_offset, record_size);
// Use the same logics as before to check whether GC is complete
if (new_free_space < _size) {

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 30, 2018

Contributor

Still racy - while you were meditating lots of other people could have used the new area, and you might now turn out to be the person who is expected to actually do the GC on that area! (record_offset < _size). I still think goto 634 or its logical equivalent is required.

If you really have a philosophical objection to goto, then put the entire block in a do {} while (0 or something) and call it continue instead. But I think goto retry is fine.

This comment has been minimized.

@davidsaada

davidsaada Jan 31, 2018

Contributor

Thanks - embraced your solution (got no problems with goto - use it a lot in my code whenever needed).

return NVSTORE_NOT_FOUND;
}
// We only have issues if we read during GC, so shared lock is required.
_write_lock.shared_lock();

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 30, 2018

Contributor

Yes, but shouldn't really be called write_lock any more. It's I guess it's a "layout" lock? Or a "gc" lock? (And if I'm still being that picky, it's a mutex).

This comment has been minimized.

@davidsaada

davidsaada Jan 31, 2018

Contributor

Hmmm... Maybe just lock...

// In the case we have crossed the limit, and the initial offset was also after the limit,
// this means we are not the first writer (uncommon case). Just wait for GC to complete.
// then retry the operation
_lock.shared_unlock();

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 31, 2018

Contributor

I think this can be simplified to

_lock.shared_unlock();
rtos::Thread::wait(MEDITATE_TIME_MS);
goto retry;

with retry moved above the shared_lock() above.

This version fails to set record_offset and has a no-longer-doing-anything for (;;)

This comment has been minimized.

@davidsaada

davidsaada Feb 1, 2018

Contributor

Right. This is the cleanest way to go.

return do_set(key, 0, NULL, DELETE_ITEM_FLAG);
}

int NVStore::init()

This comment has been minimized.

@SenRamakri

SenRamakri Feb 2, 2018

Contributor

Can you please review and see if we can optimize the stack usage of this function?

This comment has been minimized.

@davidsaada

davidsaada Feb 2, 2018

Contributor

Yes, the local variables aren't optimally arranged (alignment-wise). After arrangement they'll use 55 bytes of stack (all arrays there have two indexes). Is this problematic? Note that this is the init function, which should be called once at system life time.


#ifdef NVSTORE_TESTING

int NVStore::force_garbage_collection(void)

This comment has been minimized.

@SenRamakri

SenRamakri Feb 2, 2018

Contributor

Whys is this bound with NVSTORE_TESTING? Shouldn't we be doing garbage collection for targets smaller flash footprint/smaller NVStore as well?

This comment has been minimized.

@davidsaada

davidsaada Feb 2, 2018

Contributor

Please note that this is is not the actual garbage collection function, which is simply called garbage_collection, but the force garbage collection function, used for testing purposes.

This comment has been minimized.

@cmonr

cmonr Feb 3, 2018

Contributor

@davidsaada Why would garbage collection need to be forced durring collection? Is this the same testing as mbed test or something different?

This comment has been minimized.

@davidsaada

davidsaada Feb 3, 2018

Contributor

Actually this should be removed altogether. It was used to debug garbage collection, but it's not used for that purpose any more. Will remove it.

@@ -691,7 +691,7 @@
"macros": ["CPU_MK82FN256VDC15", "FSL_RTOS_MBED"],
"inherits": ["Target"],
"detect_code": ["0217"],
"device_has": ["ANALOGIN", "ANALOGOUT", "I2C", "I2CSLAVE", "INTERRUPTIN", "LOWPOWERTIMER", "PORTIN", "PORTINOUT", "PORTOUT", "PWMOUT", "RTC", "SERIAL", "SLEEP", "SPI", "SPISLAVE", "STDIO_MESSAGES", "TRNG"],
"device_has": ["ANALOGIN", "ANALOGOUT", "I2C", "I2CSLAVE", "INTERRUPTIN", "LOWPOWERTIMER", "PORTIN", "PORTINOUT", "PORTOUT", "PWMOUT", "RTC", "SERIAL", "SLEEP", "SPI", "SPISLAVE", "STDIO_MESSAGES", "TRNG", "FLASH"],

This comment has been minimized.

@0xc0170

0xc0170 Feb 5, 2018

Member

enabling flash for a target should be separate PR , tests provided there as well

This comment has been minimized.

@davidsaada

davidsaada Feb 5, 2018

Contributor

Right. Will be moved to a separate PR.

}
},
"target_overrides": {
"K64F": {

This comment has been minimized.

@0xc0170

0xc0170 Feb 5, 2018

Member

If a target needs to define these, and we got only 2 targets available for this feature, this PR will need to be redirected to own feature branch.

I can create one for you, feature-nvstore ?

This comment has been minimized.

@dannybenor

dannybenor Feb 5, 2018

@sg- @bulislaw @theotherjimmy We need to enhance the definition for additional targets but I think we can do it internally and do not need partners for it. We need some help with targets that we do not have. Also need the new tools from @theotherjimmy

This comment has been minimized.

@cmonr

cmonr Feb 7, 2018

Contributor

@dannybenor Which new tools are these? Is there an associated PR with them, or are they in a different repo?

This comment has been minimized.

@dannybenor

dannybenor Feb 8, 2018

@cmonr This Jira was created to follow up the tools improvements https://jira.arm.com/browse/IOTMORF-1991
For managed BL mode, the addresses are outputs.

This comment has been minimized.

@cmonr

cmonr Feb 13, 2018

Contributor

@dannybenor Sounds good, but I think @0xc0170's question is still unanswered. At this time, this feature is still only targeting a few targets, instead of the entire ecosystem.

This comment has been minimized.

@davidsaada

davidsaada Feb 13, 2018

Contributor

Going to add support for a whole lotta boards in this PR, hopefully soon.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 8, 2018

@davidsaada, if you haven't rebased this PR in a while, I would suggest doing so. TravisCI keeps failing for this PR, and I think it's because this PR is missing a particular commit.

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Feb 12, 2018

Thanks @cmonr - rebasing indeed fixed the Travis issue.
Still fails on Jenkins, but not sure why, or if it's related to the PR itself.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 12, 2018

Something went wrong with rebase , please look at the commits there (they are duplicated)

@davidsaada davidsaada force-pushed the davidsaada:david_sotp_cpp branch from c00acc4 to 2aa08c5 Feb 12, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 13, 2018

@0xc0170 The commits look alright. They look the same at a glance due to the way the commit was formatted, and how GitHub is collapsing the commit.

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Feb 13, 2018

@cmonr - The commits are OK now. Fixed them thanks to @0xc0170 comment (my rebase was problematic - fixed it and performed a force push).
TravisCI still fails on the K82F board. The build command also failed for me locally, but after a clean build it stopped failing. Does TravisCI perform a clean build?

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 13, 2018

@davidsaada, TravisCI and Jenkins both do clean builds, and they also build and test separate things.

It looks like TravisCI is about to pass (wohoo!), but Jenkins builds are still failing, due to build failures with mbed-os-cliapp (it's a private repo, and I'm actually confused as to why it's a part of PR build checks).

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Feb 27, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 27, 2018

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Feb 27, 2018

<sigh> Now there's a problem with the ARM compiler licenses. Will try invoke the morph build later.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 27, 2018

@davidsaada This isn't the only PR being hit with ARM license server issues. I can restart the build once it looks like the dust has settled.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 27, 2018

Fingers crossed: /morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 27, 2018

Build : SUCCESS

Build number : 1291
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5900/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

"help": "Area 2 size"
}
}
}

This comment has been minimized.

@LiyouZhou

LiyouZhou Feb 28, 2018

can we have some target defaults in here?

This comment has been minimized.

@davidsaada

davidsaada Feb 28, 2018

Contributor

By design, there are no target defaults in mbed_lin.json. By default, we take the last two sectors of the flash and use them for NVStore, one per area. If they are smaller than 4KB, we take a few consecutive ones to form areas >= 4KB. One can set values in mdeb_lib.json to override this default behavior, but that's entirely up to the specific application.

AnotherButler added some commits Mar 1, 2018

Copy edit nvstore.h
Copy edit for typos and minor spelling nits.
Edit README.md
Edit README with changes from docs site and minor formatting changes.
@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Mar 1, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Mar 1, 2018

Build : SUCCESS

Build number : 1313
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5900/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 1, 2018

SUCCESS exporting mbed-os-example-mesh-minimal UBLOX_EVK_ODIN_W2 gnuarmeclipse
Building mbed-os-example-mesh-minimal UBLOX_EVK_ODIN_W2 gnuarmeclipse
==========STDOUT==========

Create.
Opening 'mbed-os-example-mesh-minimal'.
Refreshing '/mbed-os-example-mesh-minimal'.

==========STDERR==========
Eclipse:
An error has occurred. See the log file
/tmp/tmpS2Dn48/.metadata/.log.

SUCCESS
FAILURE

/morph export-build

@mbed-ci

This comment has been minimized.

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Mar 1, 2018

@cmonr Thanks for triggering the exporter. Looks like morph test has been manually aborted.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 1, 2018

Good eye. I'll re-trigger it.
/morph test

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 1, 2018

@davidsaada Also, as a reminder, only the maintainers should be triggering the morph commands, especially being this close to a release PR being made.

@cmonr cmonr removed the ready for merge label Mar 1, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 1, 2018

@mbed-ci

This comment has been minimized.

@cmonr cmonr merged commit 2532196 into ARMmbed:master Mar 2, 2018

19 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the needs: review label Mar 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment