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

Correctly include EventQueue.h #9801

Merged
merged 1 commit into from Mar 17, 2019

Conversation

Projects
None yet
8 participants
@vmedcy
Copy link
Contributor

commented Feb 21, 2019

Description

There are two EventQueue.h in mbed-os codebase:
events/EventQueue.h
features/FEATURE_BLE/ble/pal/EventQueue.h

By accident, mbed compile generates includes.txt with the correct
order of include search paths. This is not the case for the CMake
exporter: targets with FEATURE_BLE enables fail to compile with errors:

mbed-os/features/cellular/framework/AT/ATHandler.h:99:60: error: 'events' has not been declared

Update all places to always include either "events/EventQueue.h"
or "ble/pal/EventQueue.h": to always find the correct header.

Pull request type

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

Reviewers

Release Notes

Correctly include EventQueue.h
There are two EventQueue.h in mbed-os codebase:
events/EventQueue.h
features/FEATURE_BLE/ble/pal/EventQueue.h

By accident, `mbed compile` generates includes.txt with the correct
order of include search paths. This is not the case for the CMake
exporter: targets with FEATURE_BLE enables fail to compile with errors:

mbed-os/features/cellular/framework/AT/ATHandler.h:99:60: error:
'events' has not been declared

Update all places to always include either "events/EventQueue.h"
or "ble/pal/EventQueue.h": to always find the correct header.
@vmedcy

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

To reproduce, export to cmake any target with "features": ["BLE"] and build:

mbed export -i cmake_gcc-arm -m CY8CKIT_062_WIFI_BT
mkdir BUILD/cmake && cd BUILD/cmake
cmake ../../ -GNinja
ninja

typical C++ error:

In file included from mbed-os/features/cellular/framework/AT/AT_CellularBase.h:20:0,
                 from mbed-os/features/cellular/framework/AT/AT_CellularStack.h:21,
                 from mbed-os/features/cellular/framework/targets/QUECTEL/M26/QUECTEL_M26_CellularStack.h:21,
                 from mbed-os/features/cellular/framework/targets/QUECTEL/M26/QUECTEL_M26_CellularStack.cpp:19:
mbed-os/features/cellular/framework/AT/ATHandler.h:74:31: error: 'events' has not been declared
     ATHandler(FileHandle *fh, events::EventQueue &queue, uint32_t timeout, const char *output_delimiter, uint16_t send_delay = 0);
                               ^~~~~~
mbed-os/features/cellular/framework/AT/ATHandler.h:74:50: error: expected ',' or '...' before '&' token
     ATHandler(FileHandle *fh, events::EventQueue &queue, uint32_t timeout, const char *output_delimiter, uint16_t send_delay = 0);
                                                  ^
In file included from mbed-os/features/cellular/framework/AT/AT_CellularBase.h:20:0,
                 from mbed-os/features/cellular/framework/AT/AT_CellularStack.h:21,
                 from mbed-os/features/cellular/framework/targets/QUECTEL/M26/QUECTEL_M26_CellularStack.h:21,
                 from mbed-os/features/cellular/framework/targets/QUECTEL/M26/QUECTEL_M26_CellularStack.cpp:19:
mbed-os/features/cellular/framework/AT/ATHandler.h:99:60: error: 'events' has not been declared
     static ATHandler *get_instance(FileHandle *fileHandle, events::EventQueue &queue, uint32_t timeout,
                                                            ^~~~~~
mbed-os/features/cellular/framework/AT/ATHandler.h:99:79: error: expected ',' or '...' before '&' token
     static ATHandler *get_instance(FileHandle *fileHandle, events::EventQueue &queue, uint32_t timeout,
                                                                               ^
mbed-os/features/cellular/framework/AT/ATHandler.h:228:5: error: 'events' does not name a type; did you mean 'gets'?
     events::EventQueue &_queue;
     ^~~~~~
     gets

@cmonr cmonr requested review from geky and ARMmbed/mbed-os-core Feb 21, 2019

@cmonr cmonr added the needs: review label Feb 21, 2019

@paul-szczepanek-arm

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

I think it would be less error prone to have a unique name for the header: PalEventQueue.h (same as we have PalGap.h, PalGattClient.h). This is not a user facing header so will not break anyone's application.

@deepikabhavnani
Copy link
Contributor

left a comment

Looks good to me

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

I think it would be less error prone to have a unique name for the header: PalEventQueue.h (same as we have PalGap.h, PalGattClient.h). This is not a user facing header so will not break anyone's application.

@paul-szczepanek-arm Can you send PR fixing the name ?

@vmedcy

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

@paul-szczepanek-arm, @0xc0170:

This is not a user facing header so will not break anyone's application.

Technically, there is no such thing as non-user-facing header, especially in mbed flow when ALL directories with headers are added to header search paths. I believe the correct solution for possible header name collisions is to consistently include everything as "module_name/header_name.h", and only add directories with modules to search paths. This is already done for most platform headers, but not currently enforced by the resource scanning rules.

@pan-

pan- approved these changes Feb 25, 2019

@pan-

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

@paul-szczepanek-arm Yes we can provide coherent naming for these headers. We need to discuss if we remove the Pal prefix on the header with it or add it to the headers without it. the inclusion pattern should be module/header. The main issue being IAR as the exporters doesn't work if two source files share the same name and we want shared name between source files and header files. @0xc0170 Do you know if this limitation stands with IAR 8.x ?

@vmedcy A header is considered not public if it is not part of the public API documentation. Of course technically you can include private headers in your application but that is a limitation of the tool not a desired behaviour.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

The main issue being IAR as the exporters doesn't work if two source files share the same name and we want shared name between source files and header files. @0xc0170 Do you know if this limitation stands with IAR 8.x ?

I dont, but @deepikabhavnani might or we test it.

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Feb 25, 2019

@cmonr cmonr added needs: review and removed needs: CI labels Feb 27, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

@vmedcy @paul-szczepanek-arm @pan- @deepikabhavnani @0xc0170 Do we need to take a quick look at this and determine if it needs to go to 5.12?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

@vmedcy @paul-szczepanek-arm @pan- @deepikabhavnani @0xc0170 Do we need to take a quick look at this and determine if it needs to go to 5.12?

Will get in CI when able, latest to 5.12.1

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

Ci scheduled

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 1, 2019

Test run: FAILED

Summary: 1 of 14 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_tools-test

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Mar 1, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

Info: This PR is good to go.

jenkins-ci/mbed-os-ci_tools-test was recently added, and didn't intend on being run against PRs just yet.

@0xc0170 0xc0170 added the needs: CI label Mar 12, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

Restarting CI (7 days ago the last run)

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

Aborted the job, need CI now for rc2 job asap, will restart later

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 12, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARMC5
  • jenkins-ci/mbed-os-ci_mbed2-build-ARMC5
  • jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-IAR8
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-IAR8
  • jenkins-ci/mbed-os-ci_build-ARMC6
@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

restarting CI as CI resources currently available

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 14, 2019

Test run: FAILED

Summary: 1 of 13 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter
@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

There's duplication in BLE , I dont think this relates to this PR, will restart to confirm and check how this happened

@cmonr cmonr added ready for merge and removed needs: CI labels Mar 15, 2019

cmonr added a commit to cmonr/mbed-os that referenced this pull request Mar 16, 2019

cmonr added a commit to cmonr/mbed-os that referenced this pull request Mar 16, 2019

@cmonr cmonr merged commit dbb33ef into ARMmbed:master Mar 17, 2019

28 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 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-ARMC5 Success
Details
jenkins-ci/mbed2-build-ARMC6 Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 8986 cycles
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Mar 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.