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.

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

Make ESP8266 compatible with bare metal profile #12022

Merged
merged 1 commit into from
Dec 11, 2019
Merged

Make ESP8266 compatible with bare metal profile #12022

merged 1 commit into from
Dec 11, 2019

Conversation

anttiylitokola
Copy link
Contributor

@anttiylitokola anttiylitokola commented Dec 4, 2019 •
edited
Loading

Summary of changes

Make ESP8266 driver compatible with bare metal profile.

Impact of changes

Migration actions required

Documentation

Pull request type 

[X Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@yogpan01, @kjbracey-arm, @VeijoPesonen, @AnttiKauppila

VeijoPesonen
VeijoPesonen reviewed Dec 4, 2019
View reviewed changes
components/wifi/esp8266-driver/ESP8266/ESP8266.cpp
@@ -272,14 +271,15 @@ bool ESP8266::reset(void)
continue;
}

_rmutex.lock();
Copy link
Contributor

@VeijoPesonen VeijoPesonen Dec 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Converting this as PlatformMutex would be the right solution. Scratch that. RTOS Mutex is rendered out in brametal builds. This code should be kept.

@ciarmcom
Copy link
Member

ciarmcom commented Dec 4, 2019

@anttiylitokola, thank you for your changes.
@yogpan01 @kjbracey-arm @VeijoPesonen @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

@0xc0170 0xc0170 added the release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 label Dec 5, 2019
michalpasztamobica
michalpasztamobica reviewed Dec 9, 2019
View reviewed changes
components/wifi/esp8266-driver/ESP8266/ESP8266.h
@@ -428,7 +428,6 @@ class ESP8266 {
PinName _serial_rts;
PinName _serial_cts;
rtos::Mutex _smutex; // Protect serial port access
rtos::Mutex _rmutex; // Reset protection
Copy link
Contributor

@michalpasztamobica michalpasztamobica Dec 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Why is _rmutex removed while _smutex can stay? I thought that "bare-metal" means "no RTOS", means no thread synchronisation mechanisms?

Copy link
Contributor

Choose a reason for hiding this comment

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

I helped prepare this.

_rmutex wasn't doing anything, as far as I can see. It was a remnant of the also-redundant condition variable bits for reset, which seemed to be imagining some other thread running OOB handlers.

The piece of code waiting for reset is inside _smutex, so it all happens synchronously, blocking inside that code.

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 11, 2019

started CI

@mbed-ci
Copy link

mbed-ci commented Dec 11, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 removed the needs: CI label Dec 11, 2019
@0xc0170 0xc0170 merged commit 5c8d46f into ARMmbed:master Dec 11, 2019
@teetak01
Copy link
Contributor

@0xc0170 can we target for 5.15.1 ?

cc: @JanneKiiskila

@JanneKiiskila
Copy link
Contributor

@fabiomadu @adbridge - yes, we would need this in the soonest, if possible.

AnttiKauppila
AnttiKauppila reviewed Dec 12, 2019
View reviewed changes
components/wifi/esp8266-driver/ESP8266Interface.h
@@ -30,7 +30,9 @@
#include "features/netsocket/WiFiAccessPoint.h"
#include "features/netsocket/WiFiInterface.h"
#include "platform/Callback.h"
#if MBED_CONF_RTOS_PRESENT
Copy link

Choose a reason for hiding this comment

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

Why is this needed? same MACRO guardian is already in that header file. And if the header file does not compile as such (includes rtos headers outside MACRO) those headers should be moved inside MBED_CONF_RTOS_PRESENT macro.

@adbridge
Copy link
Contributor

@0xc0170 No tests checked in this PR and no comment on Documentation? These should have been corrected before merging....

@adbridge
Copy link
Contributor

@0xc0170 can we target for 5.15.1 ?

cc: @JanneKiiskila

Any releases from 5.15 will be at the discretion of the project management as there are no 'planned' patch releases from this branch...

@adbridge adbridge added Release review required and removed release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 labels Dec 13, 2019
@adbridge
Copy link
Contributor

@bulislaw FYI

@bulislaw
Copy link
Member

bulislaw commented Jan 7, 2020

Lets pull it in the next patch. Low risk of breaking something.

@JanneKiiskila
Copy link
Contributor

Ask for 5.15.1, @adbridge .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AnttiKauppila AnttiKauppila AnttiKauppila left review comments

@0xc0170 0xc0170 0xc0170 approved these changes

@VeijoPesonen VeijoPesonen VeijoPesonen approved these changes

@michalpasztamobica michalpasztamobica michalpasztamobica approved these changes

@kjbracey kjbracey Awaiting requested review from kjbracey

@yogpan01 yogpan01 Awaiting requested review from yogpan01

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

Successfully merging this pull request may close these issues.