Skip to content

mem pool size: Fix calculation of memory pool size for portability #10706

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 1 commit into from
Jun 25, 2019
Merged

mem pool size: Fix calculation of memory pool size for portability #10706

merged 1 commit into from
Jun 25, 2019

Conversation

hugueskamba
Copy link
Collaborator

@hugueskamba hugueskamba commented May 29, 2019 •
edited
Loading

Description

Replace the hardcoded value calculation of the memory pool block size
with the RTX preprocessor macro (via a shim layer).
This is in preparation for the replacement of the direct access of RTX
functionalities in Mbed OS with an access via CMSIS.

Pull request type

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

Reviewers

@0xc0170

fixes: #9119

kjbracey
kjbracey reviewed May 29, 2019
View reviewed changes
rtos/MemoryPool.h
@@ -184,13 +184,11 @@ class MemoryPool : private mbed::NonCopyable<MemoryPool<T, pool_sz> > {
private:
osMemoryPoolId_t _id;
/* osMemoryPoolNew requires that pool block size is a multiple of 4 bytes. */
char _pool_mem[((sizeof(T) + 3) & ~3) * pool_sz];
char _pool_mem[MBED_RTOS_STORAGE_MEM_POOL_MEM_SIZE(pool_sz, sizeof(T))];
Copy link
Contributor

Choose a reason for hiding this comment

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

The change leaves the comment above unnecessary.

I do worry a bit about alignment, but maybe some C++11 alignas qualifier(s) could be added later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

e621e81

@ciarmcom ciarmcom requested review from 0xc0170 and a team May 29, 2019 15:01
@ciarmcom
Copy link
Member

@hugueskamba, thank you for your changes.
@0xc0170 @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

@adbridge
Copy link
Contributor

@hugueskamba could we rename the commit ? 'TO BE SQUASHED' is not really that meaningful.
@kjbracey-arm could you check the updates please ?

@hugueskamba
Copy link
Collaborator Author

hugueskamba commented Jun 19, 2019 •
edited
Loading

@hugueskamba could we rename the commit ? 'TO BE SQUASHED' is not really that meaningful.
@kjbracey-arm could you check the updates please ?

@adbridge It is an indication that the commit should be squashed and only the first commit should remain. There is no point in having two commits for this work as the second commit just removes the comment which should have been done when the first commit was made.

In my previous team we were using a squash and merge strategy. The merger would squash all commits marked for squashed at the point of merging. Do you have something similar as it will be pointless to have both commits instead of just one?

kjbracey
kjbracey approved these changes Jun 19, 2019
View reviewed changes
Copy link
Contributor

@kjbracey kjbracey left a comment

Choose a reason for hiding this comment

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

Generally we expect committers to do their own squashing and tidying with one or more force pushes before finalisation - maintainers will do a rebase and merge of the commits in the PR as-is.

@hugueskamba
Copy link
Collaborator Author

Generally we expect committers to do their own squashing and tidying with one or more force pushes before finalisation - maintainers will do a rebase and merge of the commits in the PR as-is.

Ah I see. Force pushes were frowned upon in my previous team. Ok I will tidy it up.

@hugueskamba
mem pool size: Fix calculation of memory pool size for portability
4f8305e 
Replace the hardcoded value calculation of the memory pool block size
with the RTX preprocessor macro (via a shim layer).
This is in preparation for the replacement of the direct access of RTX
functionalities in Mbed OS with an access via CMSIS.
@hugueskamba hugueskamba force-pushed the fix-softcoding-mem-pool-size-evaluation branch from e621e81 to 4f8305e Compare June 19, 2019 08:30
@adbridge
Copy link
Contributor

CI started

@mbed-ci
Copy link

mbed-ci commented Jun 21, 2019

Test run: FAILED

Summary: 6 of 7 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_mbed2-build-ARM
  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-IAR

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 24, 2019

CI restarted

@mbed-ci
Copy link

mbed-ci commented Jun 24, 2019

Test run: FAILED

Summary: 6 of 7 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_mbed2-build-ARM
  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-IAR

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 24, 2019

Aborted the job unfortunately as we have to reschedule one possibly rc4 PR.
We will restart this one once it is in.

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 24, 2019

Ci started

@mbed-ci
Copy link

mbed-ci commented Jun 25, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 merged commit e39aeff into ARMmbed:master Jun 25, 2019
@hugueskamba hugueskamba deleted the fix-softcoding-mem-pool-size-evaluation branch July 30, 2019 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@kjbracey kjbracey kjbracey approved these changes

Assignees
No one assigned
Labels
release-version: 5.13.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MemoryPool: using hard defined mem pool size
6 participants
@hugueskamba @ciarmcom @adbridge @mbed-ci @0xc0170 @kjbracey