I have pushed a fixup because I was getting an error return code when sending the commands, also the stack size of the event thread was too small to use printf, so I changed that configuration.

Do you have a way to test this and report in the PR ? (I have no access to AT based devices ATM).

I tested with the UBLOX sara-r410m. You can see that I registered an URC with the response of the scan, which takes several seconds. Then I issued the command to scan (which times out). Finally I get the message and it's reported as a received URC. Also immediate commands still work as usual.

2020-06-30 10:28:07,561 #  main(): This is RIOT! (Version: 2020.07-devel-1344-g205911-pr/drivers/at_isr_urc)
2020-06-30 10:28:07,561 # AT command test app
> add_urc +COPS
2020-06-30 10:28:13,028 #  add_urc +COPS
2020-06-30 10:28:13,028 # urc registered
> init 1 115200
2020-06-30 10:28:16,616 #  init 1 115200
> send AT+COPS=?
2020-06-30 10:28:22,351 #  send AT+COPS=?
2020-06-30 10:28:32,384 # Error
> 2020-06-30 10:28:51,739 #  urc received: +COPS: (2,"262 01","262 01","26201",9),,(0,1,2,3,4),(0,1,2)
send AT+CGMI
2020-06-30 10:28:59,274 # send AT+CGMI
2020-06-30 10:28:59,292 # Response (len=6): u-blox
> 

Sorry to only think about this now. I'm wondering if we could avoid the use of the event thread, since this adds another thread. Maybe that's not possible, since directly calling the callback when the URC would be done in ISR context if I understand it correctly.

since directly calling the callback when the URC would be done in ISR context if I understand it correctly.

Yeah, this is the main reason why I am using event thread. I think in this case it's ok to use these types of OS mechanisms for offloading.

That being said, another alternative could be that the user initializes the event queue and handles the even loop? Maybe even chose the behaviour using pseudomodules.

another alternative could be that the user initializes the event queue and handles the even loop?

That would move the complexity in the user code. I'm not sure that's very friendly for this kind of things. Since this part is only build when enabling the URC isr I think this is fine.

Please address my remaining comments and squash directly !

@aabadie squashed. Thanks for reviewing!

Travis failed and I can't restart it but that's not a problem since Murdock is green => GO!

Hello,

I am not sure if I am allowed to directly jump into the conversation -I am not a contributor just an ordinary RIOT user :)- but I could share a useful experience with you although the PR has just been merged:). In our FW, we communicate with ublox modems using this at driver. We have spent considerable amount of time in properly processing all emitted URCs. According to our observations by means of a logic analyzer, it's really hard to decide when the AT device will NOT send a URC. We have observed cases in which we send out an AT command and the first thing that we receive is a URC. Then the response of the command follows. It might sound weird but this is how the ublox modem behaves. That being said, I am not so confident that this implementation will guarantee to capture all emitted URCs.

I am not so confident that this implementation will guarantee to capture all emitted URCs.

@leandrolanzieri what do you think ? Did you encounter such problems with your ublox driver ?

@leandrolanzieri what do you think ? Did you encounter such problems with your ublox driver ?

Not so far, but if the behaviour is as @aaltuntas describes (having URCs in the middle of commands) it is possible that we do not catch all URCs with just the interrupt. I will keep an eye on that while I continue with the driver.

@aaltuntas do you have any suggestions for improvement here?

Our approach was not event based, we refactored at_expect_bytes to check for and process URCs at the beginning. The purpose of this was to capture the URCs that are received during the execution of at_send_cmd for example. Additionally, we followed the same procedure in at_drain. The purpose of this was to handle URCs that were received during idle times when there was no ongoing AT operation. We inserted delays of default 20ms just before at_drain in functions like at_send_cmd_get_resp. This delay is suggested by the ublox AT manual, which will give time the modem to emit any URCs.

So, keeping those in mind, I would suggest to put dev->awaiting_response flag in functions that constitute a whole AT operation like at_send_cmd_get_resp etc. rather than in every sub-function. To me, the best place is right after the at_send_cmd function. On the other hand, if it's adapted properly, your event based URC processing would be a cleaner solution than that of ours. Last but not least, my suggestions might only apply to ublox modems. I don't have the opportunity to test our modified driver against different AT devices.