Correctly include EventQueue.h #9801

vmedcy · 2019-02-21T16:07:48Z

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

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 · 2019-02-21T16:17:21Z

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

paul-szczepanek-arm · 2019-02-22T09:15:11Z

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

Looks good to me

0xc0170 · 2019-02-22T12:54:51Z

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 · 2019-02-22T13:35:58Z

@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- · 2019-02-25T08:51:45Z

@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 · 2019-02-25T09:03:05Z

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.

cmonr · 2019-03-01T02:07:15Z

@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 · 2019-03-01T12:46:36Z

@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 · 2019-03-01T12:47:36Z

Ci scheduled

mbed-ci · 2019-03-01T14:57:02Z

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

cmonr · 2019-03-05T20:15:25Z

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 · 2019-03-12T07:53:59Z

Restarting CI (7 days ago the last run)

0xc0170 · 2019-03-12T08:17:04Z

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

mbed-ci · 2019-03-12T09:28:57Z

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 · 2019-03-14T08:02:15Z

restarting CI as CI resources currently available

mbed-ci · 2019-03-14T10:14:39Z

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 · 2019-03-14T10:20:04Z

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

null

Correctly include EventQueue.h

cmonr requested review from geky and a team February 21, 2019 16:21

cmonr added the needs: review label Feb 21, 2019

paul-szczepanek-arm approved these changes Feb 22, 2019

View reviewed changes

deepikabhavnani approved these changes Feb 22, 2019

View reviewed changes

0xc0170 approved these changes Feb 22, 2019

View reviewed changes

pan- approved these changes Feb 25, 2019

View reviewed changes

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

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

0xc0170 added needs: CI release-version: 5.12.1 and removed needs: review labels Mar 1, 2019

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

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

0xc0170 removed the ready for merge label Mar 12, 2019

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

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

Merge pull request ARMmbed#9801 from null

439f47d

null

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

Merge pull request ARMmbed#9801 from vmedcy/fix-eventqueue-h

c9389b8

Correctly include EventQueue.h

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

cmonr removed the ready for merge label Mar 17, 2019

Correctly include EventQueue.h #9801

Correctly include EventQueue.h #9801

Uh oh!

Conversation

vmedcy commented Feb 21, 2019

Description

Pull request type

Reviewers

Release Notes

Uh oh!

vmedcy commented Feb 21, 2019

Uh oh!

paul-szczepanek-arm commented Feb 22, 2019

Uh oh!

deepikabhavnani left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Feb 22, 2019

Uh oh!

vmedcy commented Feb 22, 2019

Uh oh!

pan- commented Feb 25, 2019

Uh oh!

0xc0170 commented Feb 25, 2019

Uh oh!

cmonr commented Mar 1, 2019

Uh oh!

0xc0170 commented Mar 1, 2019

Uh oh!

0xc0170 commented Mar 1, 2019

Uh oh!

mbed-ci commented Mar 1, 2019

Test run: FAILED

Uh oh!

cmonr commented Mar 5, 2019

Uh oh!

0xc0170 commented Mar 12, 2019

Uh oh!

0xc0170 commented Mar 12, 2019

Uh oh!

mbed-ci commented Mar 12, 2019

Test run: FAILED

Uh oh!

NirSonnenschein commented Mar 14, 2019

Uh oh!

mbed-ci commented Mar 14, 2019

Test run: FAILED

Uh oh!

0xc0170 commented Mar 14, 2019

Uh oh!

Uh oh!