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

Avoid lock collision b/w SerialBase & UARTSerial #4538

Merged
merged 1 commit into from
Jun 26, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 30 additions & 18 deletions drivers/UARTSerial.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

#if DEVICE_SERIAL
#if (DEVICE_SERIAL && DEVICE_INTERRUPTIN)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems a bit funny to add this check here? Not seeing this using the InterruptIn APIs here, which usually what this guard is used for.

There currently isn't a target that has SERIAL but not INTERRUPTIN listed in its device_has descriptor, so this won't cause immediate issues. Just seems unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

That was discussed in #4524 (issue is #4515). Interrupt is declared here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bridadan Totally agree with you. Probably I should have added this bit in commit message. UARTSerial do use InterruptIn class, but yes in agreement with you, nobody can use UARTSerial without having interrupts. But just for consistency of the system, added this flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dude, my bad totally missed it. Definitely the right time to use the flag! Good catch.


#include <errno.h>
#include "UARTSerial.h"
Expand Down Expand Up @@ -80,16 +80,16 @@ off_t UARTSerial::seek(off_t offset, int whence)

int UARTSerial::sync()
{
lock();
api_lock();

while (!_txbuf.empty()) {
unlock();
api_unlock();
// Doing better than wait would require TxIRQ to also do wake() when becoming empty. Worth it?
wait_ms(1);
lock();
api_lock();
}

unlock();
api_unlock();

return 0;
}
Expand All @@ -111,16 +111,16 @@ ssize_t UARTSerial::write(const void* buffer, size_t length)
size_t data_written = 0;
const char *buf_ptr = static_cast<const char *>(buffer);

lock();
api_lock();

while (_txbuf.full()) {
if (!_blocking) {
unlock();
api_unlock();
return -EAGAIN;
}
unlock();
api_unlock();
wait_ms(1); // XXX todo - proper wait, WFE for non-rtos ?
lock();
api_lock();
}

while (data_written < length && !_txbuf.full()) {
Expand All @@ -138,7 +138,7 @@ ssize_t UARTSerial::write(const void* buffer, size_t length)
}
core_util_critical_section_exit();

unlock();
api_unlock();

return data_written;
}
Expand All @@ -149,24 +149,24 @@ ssize_t UARTSerial::read(void* buffer, size_t length)

char *ptr = static_cast<char *>(buffer);

lock();
api_lock();

while (_rxbuf.empty()) {
if (!_blocking) {
unlock();
api_unlock();
return -EAGAIN;
}
unlock();
api_unlock();
wait_ms(1); // XXX todo - proper wait, WFE for non-rtos ?
lock();
api_lock();
}

while (data_read < length && !_rxbuf.empty()) {
_rxbuf.pop(*ptr++);
data_read++;
}

unlock();
api_unlock();

return data_read;
}
Expand Down Expand Up @@ -205,12 +205,24 @@ short UARTSerial::poll(short events) const {
return revents;
}

void UARTSerial::lock(void)
void UARTSerial::lock()
{
// This is the override for SerialBase.
// No lock required as we only use SerialBase from interrupt or from
// inside our own critical section.
}

void UARTSerial::unlock()
{
// This is the override for SerialBase.
}

void UARTSerial::api_lock(void)
{
_mutex.lock();
}

void UARTSerial::unlock(void)
void UARTSerial::api_unlock(void)
{
_mutex.unlock();
}
Expand Down Expand Up @@ -262,4 +274,4 @@ void UARTSerial::tx_irq(void)

} //namespace mbed

#endif //DEVICE_SERIAL
#endif //(DEVICE_SERIAL && DEVICE_INTERRUPTIN)
27 changes: 17 additions & 10 deletions drivers/UARTSerial.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

#include "platform/platform.h"

#if DEVICE_SERIAL
#if (DEVICE_SERIAL && DEVICE_INTERRUPTIN) || defined(DOXYGEN_ONLY)

#include "FileHandle.h"
#include "SerialBase.h"
Expand Down Expand Up @@ -58,7 +58,7 @@ class UARTSerial : private SerialBase, public FileHandle {
/** Write the contents of a buffer to a file
*
* @param buffer The buffer to write from
* @param size The number of bytes to write
* @param length The number of bytes to write
* @return The number of bytes written, negative error on failure
*/
virtual ssize_t write(const void* buffer, size_t length);
Expand All @@ -72,17 +72,11 @@ class UARTSerial : private SerialBase, public FileHandle {
* * If any data is available, call returns immediately
*
* @param buffer The buffer to read in to
* @param size The number of bytes to read
* @param length The number of bytes to read
* @return The number of bytes read, 0 at end of file, negative error on failure
*/
virtual ssize_t read(void* buffer, size_t length);

/** Acquire mutex */
virtual void lock(void);

/** Release mutex */
virtual void unlock(void);

/** Close a file
*
* @return 0 on success, negative error code on failure
Expand Down Expand Up @@ -159,6 +153,18 @@ class UARTSerial : private SerialBase, public FileHandle {

private:

/** SerialBase lock override */
virtual void lock(void);

/** SerialBase unlock override */
virtual void unlock(void);

/** Acquire mutex */
virtual void api_lock(void);

/** Release mutex */
virtual void api_unlock(void);

/** Software serial buffers
* By default buffer size is 256 for TX and 256 for RX. Configurable through mbed_app.json
*/
Expand Down Expand Up @@ -191,8 +197,9 @@ class UARTSerial : private SerialBase, public FileHandle {
void wake(void);

void dcd_irq(void);

};
} //namespace mbed

#endif //DEVICE_SERIAL
#endif //(DEVICE_SERIAL && DEVICE_INTERRUPTIN) || defined(DOXYGEN_ONLY)
#endif //MBED_UARTSERIAL_H