-
Notifications
You must be signed in to change notification settings - Fork 514
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
New cloud event API #2840
base: develop
Are you sure you want to change the base?
New cloud event API #2840
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I mostly looked at the application API for large publishes.
} | ||
assert(file_); | ||
CHECK(seekInFile(fs.instance(), file_.get(), p - maxHeapSize_)); | ||
size_t n = CHECK_FS(lfs_file_write(fs.instance(), file_.get(), data, bytesToWrite)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: make sure that lfs_file_write can handle writing large buffers (>1000 bytes) in one shot or split the writes in here to the limit of lfs_file_write
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avtolstoy Is this still something on your team's radar or was it perhaps fixed already? It's that problem with writing to external flash when the source buffer is in internal flash, possibly at an unaligned address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a fix for this, should get a PR out today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
* | ||
* @return Event data. | ||
*/ | ||
Variant dataAsVariant() /* FIXME: const */; // TODO: Rename? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still relevant since we only support receiving binary payloads from large events?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly added it for feature parity with the existing API. The user can still send CBOR data in a binary event and use this method to parse it. Let me know if we don't want to support encoding and decoding variants in CloudEvent
directly and I'll remove the related methods from it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 6b4d489.
* | ||
* @return `true` if the event is being sent to the Cloud, otherwise `false`. | ||
*/ | ||
bool sending() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no shorthand to see if a CloudEvent is in NEW state. Can you add a new()
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new
is a reserved keyword so it will be isNew()
. For consistency, I'll need to rename all the other shorthand methods to have an is
prefix which I originally omitted for brevity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or rename the initial state PENDING
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added isNew
and renamed the other methods to isSending
, isOk
, etc. PENDING
is good but may be confusing for events in progress. Also, incoming events are received in that state too.
* | ||
* @return `true` if the event is valid, otherwise `false`. | ||
*/ | ||
bool valid() const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is odd since it checks if the state is not INVALID. Can you make this method invalid()
so it's symmetrical with all the other state helpers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to keep it this way so that it's consistent with other APIs unless you feel strongly about it. We only use valid()
or isValid()
everywhere else in Device OS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, sounds good
* | ||
* This method has no effect if the event is not currently being sent to the Cloud. | ||
* | ||
* A cancelled event cannot be published again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still true? What state does a cancelled event go to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, a cancelled event gets invalidated. Making it transition to a failed state instead is possible and would be more user friendly but it will require some additional work:
device-os/wiring/src/spark_wiring_cloud_event.cpp
Lines 415 to 417 in 7c89895
// TODO: For now, transition to an invalid state as an event in a failed state can be sent again | |
// and that would create a race condition between cancellation and normal completion of the event | |
// in sendComplete() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. No need for additional work here.
} | ||
int r = coap_write_payload(payload, data, size, d_->pos, nullptr /* reserved */); | ||
if (r < 0) { | ||
if (r == Error::COAP_TOO_LARGE_PAYLOAD) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way for an application to know how much room is left on the event (or what's the maximum total size of a large event) before calling write and getting COAP_TOO_LARGE_PAYLOAD?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The maximum supported size is exposed as CloudEvent::MAX_SIZE
. The app can check how much space is left like this:
if (event.pos() + 100 <= CloudEvent::MAX_SIZE) {
event.write(data, 100);
}
} | ||
size_t size = this->size(); | ||
if (!RateLimiter::instance().take(size)) { | ||
return Error::LIMIT_EXCEEDED; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So exceeding the amount of data in flight will return LIMIT_EXCEEDED, but the event will stay in the NEW state so it can be sent again later. Is there a way to query how much data is in flight to know when a new event can be sent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the event will stay in the NEW state
That was an oversight. The event will transition to a failed state on this error as well.
Is there a way to query how much data is in flight to know when a new event can be sent?
There's canPublish()
:
if (CloudEvent::canPublish(3000)) {
Particle.publish(CloudEvent().name("my_event").data(buf, 3000));
}
I was hesitant to define it as Particle.canPublish()
as not to create confusion since we have different limits for the new and old APIs.
…AP No-Response option
6de9504
to
7841734
Compare
Description
This PR introduces a new API for publishing and subscribing to cloud events. The new API supports sending and receiving up to 16K of data in an event.
Examples
Publishing a text event:
Publishing a binary-encoded event:
Using the event instance as a regular stream:
Polling the status of an event:
Using a callback to receive status updates for an event:
Subscribing to events: