Skip to content

Move BlockDevice classes inside mbed namespace #7663

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

Merged
merged 2 commits into from
Aug 8, 2018

Conversation

deepikabhavnani
Copy link

@deepikabhavnani deepikabhavnani commented Jul 31, 2018

Description

Move BlockDevice classes inside mbed namespace. Resolves: #6684

Dependent on PR: ##7660

Pull request type

[X] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

@deepikabhavnani
Copy link
Author

Will resolve conflict and rebase once #7660 is in.

@deepikabhavnani
Copy link
Author

Rebased + preceding PR merged

Copy link
Contributor

@davidsaada davidsaada left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Contributor

@geky geky left a comment

Choose a reason for hiding this comment

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

Looks good to me too 👍

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 6, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Aug 6, 2018

Build : FAILURE

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

@deepikabhavnani
Copy link
Author

trying build after adding missing header file

/morph build

@cmonr
Copy link
Contributor

cmonr commented Aug 7, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Aug 7, 2018

Build : FAILURE

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

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 7, 2018

Build failure indicates missing header file inclusion (I found at least memset there)

@cmonr
Copy link
Contributor

cmonr commented Aug 7, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Aug 7, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented Aug 7, 2018

@mbed-ci
Copy link

mbed-ci commented Aug 7, 2018

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 9, 2018

This should definitely be up for the next feature release (I would go for major if this breaks whoever includes just blockdevice and use it without mbed.h inclusion in the code - will require good documentation to migrate the code !), not patch. Still would like to check how to mitigate adding the namespace to BlockDevice.

FATFileSystem is not yet in the mbed namespace (isn't this also our implementation) ? Intention there ?

@deepikabhavnani deepikabhavnani deleted the namespace_bd_update branch August 9, 2018 13:58
@deepikabhavnani
Copy link
Author

FATFileSystem is not yet in the mbed namespace (isn't this also our implementation) ? Intention there ?

FatFileSystem is our implementation but it used ChanFs so was not sure if we can have it inside mbed namespace. Same is true for LittleFileSystem

@deepikabhavnani
Copy link
Author

will require good documentation to migrate the code !),

@0xc0170 @AnotherButler - Please suggest the good place to capture this change. All our examples use "mbed.h" and since class reference is directly pulled from code it will have namespace in reference.

@geky
Copy link
Contributor

geky commented Aug 9, 2018

Both FATFileSystem and LittleFileSystem should be inside mbed namespace.

IMO All classes in mbed-os should be inside the mbed namespace. I don't know of any reasons this shouldn't be true.

@0xc0170 +1 For holding this until a minor release

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

Successfully merging this pull request may close these issues.

6 participants