Skip to content
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

FATFileSystem::stat() enabled for all compilers #11193

Merged
merged 1 commit into from Aug 13, 2019

Conversation

@michalpasztamobica
Copy link
Contributor

commented Aug 9, 2019

Description

Fixes #10198
Macro which restricted compilation to GCC_ARM is removed.
Existing read_write() test is amended to call stat() and check that correct size is returned.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@SeppoTakalo
@geky
@kjbracey-arm

FATFileSystem::stat() enabled for all compilers
Macro which restricted compilation to GCC_ARM is removed.
Existing read_write() test is amended to call stat() and check that correct size is returned.

@ciarmcom ciarmcom requested review from geky, kjbracey-arm, SeppoTakalo and ARMmbed/mbed-os-maintainers Aug 9, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

@0xc0170
0xc0170 approved these changes Aug 9, 2019
@kjbracey-arm
Copy link
Contributor

left a comment

Can you check if this breaks ARMC5? You might want to have a #ifndef __CC_ARM there for that.

Despite lack of official support or testing, if it's easy to avoid breakage, we should.

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@kjbracey-arm , I checked that the proposed changes compile fine with:

Product: DS-5 Ultimate Edition 5.29.2
Component: ARM Compiler 5.06 update 3 (build 300)

and the test (as modified in this review) passed.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

Cool. I wonder why it was originally thought not to work on ARMCC then?

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

To my understanding the reasons were twofold: 1) not enough testing to be sure that this worked 2) even if the testing was there and indicated an issue a couple of years ago, then both IAR and ARM compiler improved since then - the #ifdef is 3 years old and might have been copied from an even older location, unchanged.

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Aug 12, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

CI restarted

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 12, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 merged commit b82cfcc into ARMmbed:master Aug 13, 2019

26 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/jenkins/pr-merge Manually fixed status
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8595 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.