Skip to content
This repository has been archived by the owner on Jan 3, 2020. It is now read-only.

Add optional polling interval to reduce CPU usage #113

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Qowy
Copy link

@Qowy Qowy commented Feb 24, 2017

As soon as Firmata.begin is called one of the cores of my Raspberry Pi 3 is fully utilized.
While this might be necessary for highly latency dependent applications, it seems like total overkill for basic usage of the firmata protocol.

This Pull Request introduces a second UwpFirmata(ulong pollingInterval) constructor that sleeps the inputThread() for the given amount of micro seconds.

A value of for example 10ms reduces the CPU usage to non noticable levels and is still entirely sufficent for simple non realtime data polling vie SYSEX

@msftgits
Copy link

Hi, I am closing and re-opening this PR to bump the CLA bot. Sorry for the inconvenience!

@ooeygui
Copy link
Member

ooeygui commented Feb 24, 2017

Hi @Qowy,
This is a great change. That thread should definitely not be busy waiting. This file uses spaces for indent, could you match it with your change? (the lines you added are tab indented).

@ooeygui
Copy link
Member

ooeygui commented Feb 24, 2017

I wonder if that read should block....

@Qowy
Copy link
Author

Qowy commented Feb 24, 2017

I updated the commit and changed the tabs into spaces.

As to whether it should block:

inputThread() calls processInput() which eventually calls IStream->read()

I only looked at the UsbSerial implementation of it:

read() calls available() which in essence calls LoadAsync() on the Windows::Storage::Streams::DataReader when there is no data available.
I am not too well versed in the C++ async stuff but as far as I understand:

In order to make it truly async the create_task(LoadAsync()).then(//callback) pattern would have to be used. However I cannot see how this would work with the current structure of the code.

Currently it just polls the IAsyncTask returned by LoadAsync() on whether it is running and if the underlying DataReader has pending input.

if (!connectionReady()) {
        return 0;
    }
    
if (_rx->UnconsumedBufferLength) {
	if (_rx->UnconsumedBufferLength > 0xFFFF) { return 0xFFFF; }
	return _rx->UnconsumedBufferLength;
 }
else if (_current_load_operation->Status != Windows::Foundation::AsyncStatus::Started) {
	// Attempt to detect disconnection
	if (_current_load_operation->Status == Windows::Foundation::AsyncStatus::Error)
	{
		_connection_ready = false;
		ConnectionLost(L"A fatal error has occurred in UsbSerial::read() and your connection has been lost.");
		return 0;
	}

	_current_load_operation = _rx->LoadAsync(MAX_READ_SIZE);
}

return 0;

Considering this, this pull request seems like the only possible solution without rewriting the entire ISerial and UWPFirmata classes.

However maybe I am misunderstanding the whole async pattern in C++ and there is a simple solution in there somewhere.

@Qowy
Copy link
Author

Qowy commented Feb 25, 2017

In short I think as long as IStream->Read() in Microsoft.Maker.Serial is not implemented as a IAsyncOperation<uint16_t> (maybe I will take a look at this if it is feasible but I can't say if I have the time) this pr is the only solutions since Microsoft.Maker.Firmata is essentially consuming a synchronous API

@Qowy
Copy link
Author

Qowy commented Feb 25, 2017

See #114 for centralized discussion about possible solutions.

@ms-iot ms-iot deleted a comment from msftclas Sep 26, 2017
@ms-iot ms-iot deleted a comment from msftclas Sep 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants