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

Add the INTERRUPTIN compilation guard for ESP8266 #10244

Merged
merged 1 commit into from Mar 29, 2019

Conversation

Projects
None yet
9 participants
@michalpasztamobica
Copy link
Contributor

commented Mar 27, 2019

Description

In case INTERRUPTIN flag is not set for a particular target, ESP8266 source code should not be compiled. The reason is that it relies on submodules (such as UARTSerial), which will not compile without INTERRUPTIN flag set. Thus, the compilation fails with an error without the flag.

The solution to the issue is to guard the ESP8266 with a macro checking that INTERRUPTIN flag is set.

This fixes #10061 .

Pull request type

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

Reviewers

@SeppoTakalo
@VeijoPesonen

@ciarmcom ciarmcom requested review from SeppoTakalo, VeijoPesonen and ARMmbed/mbed-os-maintainers Mar 27, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

@cmonr

cmonr approved these changes Mar 27, 2019

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:esp8266_interruptin branch from db6e7fd to aad6539 Mar 28, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

Travis made me fix some pending astyle issues, hence the small update.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:esp8266_interruptin branch from aad6539 to 5dbaa40 Mar 28, 2019

@cmonr cmonr added needs: CI and removed needs: review labels Mar 28, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 29, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test
@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

One of the test suites for K64F timed out. I doubt this is related to my changes...

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

restarted greentea test

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

@michalpasztamobica please add a summary of the issue and the solution to the Description , rather than just 'Fixes #xxx' , thanks.

@adbridge adbridge added needs: work and removed needs: CI labels Mar 29, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

@adbridge , you are right. I am sorry, the description has been updated.

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

@adbridge , you are right. I am sorry, the description has been updated.

Thanks :)

@adbridge adbridge merged commit f7c289b into ARMmbed:master Mar 29, 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 9933 cycles (+942 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 (+0.00%)
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
@@ -893,7 +893,7 @@ nsapi_error_t ESP8266Interface::set_country_code(bool track_ap, const char *coun

// Firmware takes only first three characters
strncpy(_ch_info.country_code, country_code, sizeof(_ch_info.country_code));
_ch_info.country_code[sizeof(_ch_info.country_code)-1] = '\0';
_ch_info.country_code[sizeof(_ch_info.country_code) - 1] = '\0';

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Apr 5, 2019

Member

For future - styling changes should be separate (I got to this line because its causing merge errors when applying this patch - someone else changed this line elsewhere)

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Apr 5, 2019

Member

Same for the line above, these 2 are causing conflicts when doing new release candidate

This comment has been minimized.

Copy link
@michalpasztamobica

michalpasztamobica Apr 5, 2019

Author Contributor

@0xc0170 , just to make sure - you mean "separate commit", not "separate PR"?

This comment has been minimized.

Copy link
@michalpasztamobica

michalpasztamobica Apr 5, 2019

Author Contributor

I was forced to change this line by the Travis astyle check, it is likely that another PR faced the same issue.
No idea how astyle missed this when it was committed first, but it is checking the whole file that is being comitted :(
Would it help if the change was in a separate commit?

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Apr 5, 2019

Member

correct separate commit at least. I've identified this, 2 PR have t he same changes - #10265

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Apr 5, 2019

Member

Still having issues with this patch - changing the style for a commit that is targeting 5.13 (country code). I'll move this to 5.13 . Please send separate patch for 5.12 (targeting 5.12 branch) to be included in 5.12.2 release. We will patch manually.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

Set to 5.13 for now, new PR should be created for 5.12.2 release (or 5.12.1 but would be need to be created today). I'll continue fighting other conflicts.

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

@0xc0170 #10329 created - same changes but no astyle.

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.