In getting my product working with the Photon 2, I've noticed that network operations take longer than they did on the Photon 1. This wouldn't be too much of an issue by itself, but the rest of the embedded side expects a response within 500 ms before it halts motor control. During connection, I've consistently seen delays of 600-1000 ms, causing my system to stall if it was moving.
To address this, I've made a simple thread that runs the serial read/write code that I'd normally be running in my loop.
void setup() {
...
os_mutex_create(&mutex);
new Thread("serialThread", serialThread, OS_THREAD_PRIORITY_NETWORK_HIGH);
}
void loop(){
...
serialProcessing();
}
static void serialThread(void) {
system_tick_t lastThreadTime = 0;
while(true) {
// If we've written in the past 100 ms, do nothing
if (millis() - waiting_since >= 100) {
serialProcessing();
Log.info("serialThread ran");
}
// Delay so we're called every 100 milliseconds (10 times per second)
os_thread_delay_until(&lastThreadTime, 100);
}
}
static void serialProcessing(void) {
os_mutex_lock(mutex);
readSerial1Stuff();
writeSerial1Stuff();
waiting_since = millis();
os_mutex_unlock(mutex);
}
0000007054 [ncp.rltk.client] INFO: Try to connect to ssid: [ssid] (b0:19:21:08:56:34)
0000010947 [app] INFO: serialThread ran
0000011147 [app] INFO: serialThread ran
0000011347 [app] INFO: serialThread ran
0000011530 [app] INFO: LOOP TOOK A WHILE: top->top[701] end->top[0] ms
0000011545 [app] INFO: serialThread ran
0000011650 [hal] INFO: mbedtls_platform_set_calloc_free
0000011847 [app] INFO: serialThread ran
0000012045 [app] INFO: serialThread ran
0000012160 [app] INFO: LOOP TOOK A WHILE: top->top[511] end->top[0] ms
0000012246 [app] INFO: serialThread ran
0000012446 [app] INFO: serialThread ran
0000012645 [app] INFO: serialThread ran
0000012672 [app] INFO: LOOP TOOK A WHILE: top->top[512] end->top[0] ms
My hangup is that I needed to give it a priority of OS_THREAD_PRIORITY_NETWORK_HIGH for it to preempt whatever the networking stuff the Device OS is doing. Anything less than that and it will just wait the same amount of time as the rest of the loop. So then for my question... is this a good idea? It seems to connect to the cloud fine. And since my code during normal operation is writing often, the thread won't need to process much (but it may still preempting something at the network priority).
Why are you calling serialProcessing() from both loop and the thread? If the loop thread starts, obtains the mutex, gets interrupted by the networking thread, the networking thread returns, then the higher priority serialThread will run, but then will get blocked by the loop thread mutex, which is probably not the desired behavior.
Also, I'm not 100% sure that os_thread_delay_until works correctly with equal-priority threads. It doesn't work properly on Gen 3, and it may or may not work properly with Gen 4. Using delay() is safer, though it will drift.
But once you fix those issues you can probably put the serial thread at a higher priority than networking as long as it can't block on a lower priority thread. It's not ideal, but it should work.
Great question - you're right it's probably not what I want. I want the loop to be able to handle it normally, and the thread to take over if it hasn't been serviced in too long of a period. Using a mutex how I do here here doesn't enforce that.
My current code also puts the Serial1.write() in a single threaded block (possibly causing unrelated issues).
The reasoning for it is that we want to finish sending an entire buffer over serial, and not have delays between bytes if another thread takes over. If it's in that block my thread won't be able to start, giving it time to finish before the thread takes over and does the same thing.
Also, is Serial1.read() thread safe? If not that's a reason to use a mutex here too.
I think the single threaded block will introduce more problems than it fixes. What I would probably do is:
Only do operations on serial from a high priority thread, probably two, never from loop.
Only do writes from the high priority serial write thread. All it does is grab contiguous blocks from a queue and send them. Since the thread is high priority, and won't be interrupted for at least 1 millisecond after being swapped in, it's highly likely to send all bytes without a pause. It should not have a mutex on the serial port, since it's the only thing writing and read and writes can be concurrent.
Do the reads from the thread and never from loop. Don't do serial writes from this thread; instead enqueue them to be sent from the write thread. Since this will never be blocked by the user loop thread, it should run very regularly, at least once per millisecond, which should be fast enough to service the serial port.
I can try to restructure my code with two threads and we'll see how it goes.
Would you recommend trying to block the thread writing until I have data I want to send, like in this example? This way I the thread would only run as needed and not be interrupting other threads if I understand correctly. Something like this,
void setup() {
...
os_mutex_create(&mutex);
// Grab lock initially
os_mutex_lock(mutex);
new Thread("serialWriterThread", serialWriterThread, OS_THREAD_PRIORITY_NETWORK_HIGH);
}
static void serialWriterThread(void) {
while(true) {
// Block thread until we have stuff to write
os_mutex_lock(mutex);
writeSerial1Stuff();
os_mutex_unlock(mutex);
// Delay so we're called every ~1 milliseconds (1000 times per second)
delay(1);
}
}
void loop(){
...
enqueueSerial();
}
void enqueueSerial(){
...
enqueueBytes();
// If I unlock then lock, the other thread of higher priority will immediately run, right?
// And if it doesn't because it's already run in this time slice, it will run in the next one < 1 ms later?
os_mutex_unlock(mutex);
os_mutex_lock(mutex);
}
I would use a os_queue instead of a mutex, but the end result will be the same. As long as the serial writer thread is the only thing that writes to the serial port, it doesn't need a lock if gated by a queue.
So I took your advice and used separate threads for reading and writing to serial, but also added a third thread that can parse the serial data and prepare a message to send. That third thread only runs if the loop has been blocked for over 100 ms. So now reading/writing is only done by their respective threads and I can call serialProcessing() from by my loop and from my thread, if needed. Everything seems to work well and I was able to get rid of the single threaded block!
void setup() {
...
// The only element in the queue is the send size
os_queue_create(&writeQueue, sizeof(int), 1, nullptr);
new Thread("serialReadThread", serialReadThread, OS_THREAD_PRIORITY_NETWORK_HIGH);
new Thread("serialWriteThread", serialWriteThread, OS_THREAD_PRIORITY_NETWORK_HIGH);
new Thread("serialProcessingThread", serialProcessingThread, OS_THREAD_PRIORITY_NETWORK_HIGH);
}
void loop(){
...
serialProcessing();
...
}
static void serialProcessing(void) {
// If receive buffer may have a message
if (attempt_parse){
parseReceiveBuffer();
}
// Respond to embedded device messages (calls send())
createDeviceResponse();
}
void send(int length){
int bytes = generatePacket(length);
if (bytes){
//Allow the serial write thread to take care of sending the bytes
os_queue_put(writeQueue, &bytes, 0, nullptr);
}
}
// If our loop is blocked for more than 100 ms, this thread will process data instead
static void serialProcessingThread(void) {
while(true) {
// If we've written in the past 100 ms, do nothing
if (millis() - waiting_since >= 100) {
serialProcessing();
Log.info("serialProcessingThread ran");
}
// Delay so we're called 100 ms later (~10 times per second)
delay(100);
}
}
void serialReadThread(void) {
system_tick_t lastThreadTime = 0;
while(true) {
// Get serial data from the device and add it to the buffer
getSerial1Data();
// Delay so we're called every 1 ms (1000 times per second)
os_thread_delay_until(&lastThreadTime, 1);
}
}
void serialWriteThread(void) {
system_tick_t lastThreadTime = 0;
int bytes;
while(true) {
// Take number of bytes to send from the queue, blocking until there are bytes to send
if (!os_queue_take(writeQueue, &bytes, CONCURRENT_WAIT_FOREVER, nullptr)) {
Serial1.write(send_buffer, bytes);
awaiting_response = true;
waiting_since = millis();
}
// Delay so we're called every 1 ms (1000 times per second)
os_thread_delay_until(&lastThreadTime, 1);
}
}