+1, I like the simplification and the idea of NMP header being in one place. I had a few doubts which I clarified with Chris.
Regards, Vipul Rahane > On Mar 2, 2017, at 8:17 AM, [email protected] wrote: > > MYNEWT-647 Changes to NMP over OIC scheme > > This commit addresses the following three issues with NMP over OIC (OMP): > > 1. Parts of NMP header missing from OMP responses. This causes problems > when multiplexing OMP among many target devices. > > 2. Parts of the NMP header are scattered in a few different places. > This works, but it makes it difficult to debug packet traces. > > 3. NMP errors get lost in translation. Instead of an NMP message with > an error code, the OMP server sends back a generic "Bad Request" OIC > message. > > ***** Current Scheme > > *** Requests > > The NMP op is indicated by the OIC op: > NMP read <=> OIC get > NMP write <=> OIC put > > The NMP group and ID are indicated as CoAP URI Query options: > > gr=<group> > id=<id> > > The remaining NMP header fields, seq and flags, are not present. > > The NMP payload is the entire CoAP request body. This is identical to the body > of a plain NMP request. > > *** Responses > > The NMP header is not present. The NMP op is indicated in the payload (see > below), but other header fields cannot be inferred. > > Payload consists of a single CBOR key-value pair. For read responses, the key > is "r"; for write responses, the key is "w". The value is a second CBOR map > containing the actual NMP response fields. > > Errors encountered during processing of NMP requests are reported via OIC > error > responses (bad request, internal server error). > > ***** Proposed Scheme > > *** Requests > > * The OIC op is always the same: put. > > * No URI Query CoAP options. > > * The NMP header is included in the payload as a key-value pair (key="_h"). > This pair is in the root map of the request and is a sibling of the other > request fields. The value of this pair is the big-endian eight-byte NMP > header with a length field of 0. > > *** Responses > > * As with requests, the NMP header is included in the payload as a > key-value pair (key="_h"). > > * No "r" or "w" field. The response fields are inserted into the root map > as a sibling of the "_h" pair. > > * Errors encountered during processing of NMP requests are reported > identically to other NMP responses (embedded NMP response). > > *** Notes > > * Keys that start with an underscore are reserved to the OIC manager > protocol (e.g., "_h"). NMP requests and responses must not name any of > their fields with a leading underscore. > > > Project: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/repo > Commit: > http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/commit/860d2d26 > Tree: > http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/tree/860d2d26 > Diff: > http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/diff/860d2d26 > > Branch: refs/heads/develop > Commit: 860d2d266110918b4a6f923f1e18c9032d13c07d > Parents: a4df93c > Author: Christopher Collins <[email protected]> > Authored: Wed Mar 1 17:41:21 2017 -0800 > Committer: Christopher Collins <[email protected]> > Committed: Thu Mar 2 08:15:05 2017 -0800 > > ---------------------------------------------------------------------- > mgmt/mgmt/include/mgmt/mgmt.h | 24 ++- > mgmt/mgmt/src/mgmt.c | 22 ++- > mgmt/newtmgr/include/newtmgr/newtmgr.h | 21 --- > mgmt/oicmgr/src/oicmgr.c | 238 ++++++++++++++++++---------- > 4 files changed, 192 insertions(+), 113 deletions(-) > ---------------------------------------------------------------------- > > > http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/blob/860d2d26/mgmt/mgmt/include/mgmt/mgmt.h > ---------------------------------------------------------------------- > diff --git a/mgmt/mgmt/include/mgmt/mgmt.h b/mgmt/mgmt/include/mgmt/mgmt.h > index 9b79723..3c402f2 100644 > --- a/mgmt/mgmt/include/mgmt/mgmt.h > +++ b/mgmt/mgmt/include/mgmt/mgmt.h > @@ -36,6 +36,10 @@ extern "C" { > #define STR(x) #x > #endif > > +#define NMGR_OP_READ (0) > +#define NMGR_OP_READ_RSP (1) > +#define NMGR_OP_WRITE (2) > +#define NMGR_OP_WRITE_RSP (3) > > /* First 64 groups are reserved for system level newtmgr commands. > * Per-user commands are then defined after group 64. > @@ -63,6 +67,24 @@ extern "C" { > #define MGMT_ERR_EBADSTATE (6) /* Current state disallows command. */ > #define MGMT_ERR_EPERUSER (256) > > +#define NMGR_HDR_SIZE (8) > + > +struct nmgr_hdr { > +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ > + uint8_t nh_op:3; /* NMGR_OP_XXX */ > + uint8_t _res1:5; > +#endif > +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ > + uint8_t _res1:5; > + uint8_t nh_op:3; /* NMGR_OP_XXX */ > +#endif > + uint8_t nh_flags; /* XXX reserved for future flags */ > + uint16_t nh_len; /* length of the payload */ > + uint16_t nh_group; /* NMGR_GROUP_XXX */ > + uint8_t nh_seq; /* sequence number */ > + uint8_t nh_id; /* message ID within group */ > +}; > + > struct mgmt_cbuf; > > typedef int (*mgmt_handler_func_t)(struct mgmt_cbuf *); > @@ -85,7 +107,7 @@ struct mgmt_group { > sizeof(struct mgmt_handler)); > > int mgmt_group_register(struct mgmt_group *group); > -void mgmt_cbuf_setoerr(struct mgmt_cbuf *njb, int errcode); > +int mgmt_cbuf_setoerr(struct mgmt_cbuf *njb, int errcode); > const struct mgmt_handler *mgmt_find_handler(uint16_t group_id, > uint16_t handler_id); > > > http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/blob/860d2d26/mgmt/mgmt/src/mgmt.c > ---------------------------------------------------------------------- > diff --git a/mgmt/mgmt/src/mgmt.c b/mgmt/mgmt/src/mgmt.c > index 4ac232a..c1a2a1e 100644 > --- a/mgmt/mgmt/src/mgmt.c > +++ b/mgmt/mgmt/src/mgmt.c > @@ -137,14 +137,20 @@ err: > return (NULL); > } > > -void > +int > mgmt_cbuf_setoerr(struct mgmt_cbuf *cb, int errcode) > { > - CborEncoder *penc = &cb->encoder; > - CborError g_err = CborNoError; > - CborEncoder rsp; > - g_err |= cbor_encoder_create_map(penc, &rsp, CborIndefiniteLength); > - g_err |= cbor_encode_text_stringz(&rsp, "rc"); > - g_err |= cbor_encode_int(&rsp, errcode); > - g_err |= cbor_encoder_close_container(penc, &rsp); > + int rc; > + > + rc = cbor_encode_text_stringz(&cb->encoder, "rc"); > + if (rc != 0) { > + return rc; > + } > + > + rc = cbor_encode_int(&cb->encoder, errcode); > + if (rc != 0) { > + return rc; > + } > + > + return 0; > } > > http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/blob/860d2d26/mgmt/newtmgr/include/newtmgr/newtmgr.h > ---------------------------------------------------------------------- > diff --git a/mgmt/newtmgr/include/newtmgr/newtmgr.h > b/mgmt/newtmgr/include/newtmgr/newtmgr.h > index 50deb15..1cf314d 100644 > --- a/mgmt/newtmgr/include/newtmgr/newtmgr.h > +++ b/mgmt/newtmgr/include/newtmgr/newtmgr.h > @@ -29,27 +29,6 @@ > extern "C" { > #endif > > -#define NMGR_OP_READ (0) > -#define NMGR_OP_READ_RSP (1) > -#define NMGR_OP_WRITE (2) > -#define NMGR_OP_WRITE_RSP (3) > - > -struct nmgr_hdr { > -#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ > - uint8_t nh_op:3; /* NMGR_OP_XXX */ > - uint8_t _res1:5; > -#endif > -#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ > - uint8_t _res1:5; > - uint8_t nh_op:3; /* NMGR_OP_XXX */ > -#endif > - uint8_t nh_flags; /* XXX reserved for future flags */ > - uint16_t nh_len; /* length of the payload */ > - uint16_t nh_group; /* NMGR_GROUP_XXX */ > - uint8_t nh_seq; /* sequence number */ > - uint8_t nh_id; /* message ID within group */ > -}; > - > struct nmgr_transport; > > /** > > http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/blob/860d2d26/mgmt/oicmgr/src/oicmgr.c > ---------------------------------------------------------------------- > diff --git a/mgmt/oicmgr/src/oicmgr.c b/mgmt/oicmgr/src/oicmgr.c > index d2110ef..d7d3591 100644 > --- a/mgmt/oicmgr/src/oicmgr.c > +++ b/mgmt/oicmgr/src/oicmgr.c > @@ -22,6 +22,7 @@ > > #include <os/os.h> > #include <os/endian.h> > +#include <defs/error.h> > > #include <assert.h> > #include <string.h> > @@ -52,9 +53,6 @@ static struct omgr_state omgr_state = { > .os_event.ev_cb = omgr_event_start, > }; > > -static void omgr_oic_get(oc_request_t *request, oc_interface_mask_t > interface); > -static void omgr_oic_put(oc_request_t *request, oc_interface_mask_t > interface); > - > struct os_eventq * > mgmt_evq_get(void) > { > @@ -67,124 +65,199 @@ mgmt_evq_set(struct os_eventq *evq) > os_eventq_designate(&omgr_state.os_evq, evq, &omgr_state.os_event); > } > > -static const struct mgmt_handler * > -omgr_find_handler(const char *q, int qlen) > +static int > +omgr_oic_read_hdr(struct CborValue *cv, struct nmgr_hdr *out_hdr) > { > - char id_str[8]; > - int grp = -1; > - int id = -1; > - char *str; > - char *eptr; > - int slen; > - > - slen = oc_ri_get_query_value(q, qlen, "gr", &str); > - if (slen > 0 && slen < sizeof(id_str) - 1) { > - memcpy(id_str, str, slen); > - id_str[slen] = '\0'; > - grp = strtoul(id_str, &eptr, 0); > - if (*eptr != '\0') { > - return NULL; > - } > + size_t hlen; > + int rc; > + > + struct cbor_attr_t attrs[] = { > + [0] = { > + .attribute = "_h", > + .type = CborAttrByteStringType, > + .addr.bytestring.data = (void *)out_hdr, > + .addr.bytestring.len = &hlen, > + .nodefault = 1, > + .len = sizeof *out_hdr, > + }, > + [1] = { 0 } > + }; > + > + rc = cbor_read_object(cv, attrs); > + if (rc != 0 || hlen != sizeof *out_hdr) { > + return MGMT_ERR_EINVAL; > } > - slen = oc_ri_get_query_value(q, qlen, "id", &str); > - if (slen > 0 && slen < sizeof(id_str) - 1) { > - memcpy(id_str, str, slen); > - id_str[slen] = '\0'; > - id = strtoul(id_str, &eptr, 0); > - if (*eptr != '\0') { > - return NULL; > - } > + > + out_hdr->nh_len = ntohs(out_hdr->nh_len); > + out_hdr->nh_group = ntohs(out_hdr->nh_group); > + > + return 0; > +} > + > +static int > +omgr_encode_nmp_hdr(struct CborEncoder *enc, struct nmgr_hdr hdr) > +{ > + int rc; > + > + rc = cbor_encode_text_string(enc, "_h", 2); > + if (rc != 0) { > + return MGMT_ERR_ENOMEM; > + } > + > + hdr.nh_len = htons(hdr.nh_len); > + hdr.nh_group = htons(hdr.nh_group); > + > + /* Encode the NMP header in the response. */ > + rc = cbor_encode_byte_string(enc, (void *)&hdr, sizeof hdr); > + if (rc != 0) { > + return MGMT_ERR_ENOMEM; > + } > + > + return 0; > +} > + > +static int > +omgr_send_err_rsp(struct CborEncoder *enc, const struct nmgr_hdr *hdr, > + int nmp_status) > +{ > + int rc; > + > + rc = omgr_encode_nmp_hdr(enc, *hdr); > + if (rc != 0) { > + return rc; > + } > + > + rc = cbor_encode_text_stringz(enc, "rc"); > + if (rc != 0) { > + return MGMT_ERR_ENOMEM; > + } > + > + rc = cbor_encode_int(enc, nmp_status); > + if (rc != 0) { > + return MGMT_ERR_ENOMEM; > } > - return mgmt_find_handler(grp, id); > + > + return 0; > } > > static void > -omgr_oic_op(oc_request_t *req, oc_interface_mask_t mask, int isset) > +omgr_oic_put(oc_request_t *req, oc_interface_mask_t mask) > { > struct omgr_state *o = &omgr_state; > const struct mgmt_handler *handler; > uint16_t data_off; > struct os_mbuf *m; > int rc = 0; > - extern CborEncoder g_encoder; > + struct nmgr_hdr hdr; > + int rsp_hdr_filled = 0; > > - if (!req->query_len) { > - goto bad_req; > - } > - > - handler = omgr_find_handler(req->query, req->query_len); > - if (!handler) { > - goto bad_req; > - } > + coap_get_payload(req->packet, &m, &data_off); > > - rc = coap_get_payload(req->packet, &m, &data_off); > cbor_mbuf_reader_init(&o->os_cbuf.ob_reader, m, data_off); > - > cbor_parser_init(&o->os_cbuf.ob_reader.r, 0, &o->os_cbuf.ob_mj.parser, > &o->os_cbuf.ob_mj.it); > > - /* start generating the CBOR OUTPUT */ > - /* this is worth a quick note. We are encoding CBOR within CBOR, so we > - * need to use the same encoder as ocf stack. We are using their global > - * encoder g_encoder which they intialized before this function is > called. > - * But we can't call their macros here as it won't use the right mape > - * encoder (ob_mj) */ > + rc = omgr_oic_read_hdr(&o->os_cbuf.ob_mj.it, &hdr); > + if (rc != 0) { > + rc = MGMT_ERR_EINVAL; > + goto done; > + } > + > + /* Convert request header to response header to be sent. */ > + switch (hdr.nh_op) { > + case NMGR_OP_READ: > + hdr.nh_op = NMGR_OP_READ_RSP; > + break; > + > + case NMGR_OP_WRITE: > + hdr.nh_op = NMGR_OP_WRITE_RSP; > + break; > + > + default: > + goto done; > + } > + rsp_hdr_filled = 1; > + > + /* Begin root map in response. */ > cbor_encoder_create_map(&g_encoder, &o->os_cbuf.ob_mj.encoder, > CborIndefiniteLength); > > + handler = mgmt_find_handler(hdr.nh_group, hdr.nh_id); > + if (handler == NULL) { > + rc = MGMT_ERR_ENOENT; > + goto done; > + } > + > + cbor_mbuf_reader_init(&o->os_cbuf.ob_reader, m, data_off); > + cbor_parser_init(&o->os_cbuf.ob_reader.r, 0, &o->os_cbuf.ob_mj.parser, > + &o->os_cbuf.ob_mj.it); > + > switch (mask) { > case OC_IF_BASELINE: > oc_process_baseline_interface(req->resource); > + /* Fallthrough */ > + > case OC_IF_RW: > - if (!isset) { > - cbor_encode_text_string(&root_map, "r", 1); > - if (handler->mh_read) { > - rc = handler->mh_read(&o->os_cbuf.ob_mj); > + switch (hdr.nh_op) { > + case NMGR_OP_READ_RSP: > + if (handler->mh_read == NULL) { > + rc = MGMT_ERR_ENOENT; > } else { > - goto bad_req; > + rc = handler->mh_read(&o->os_cbuf.ob_mj); > } > - } else { > - cbor_encode_text_string(&root_map, "w", 1); > - if (handler->mh_write) { > - rc = handler->mh_write(&o->os_cbuf.ob_mj); > + break; > + > + case NMGR_OP_WRITE_RSP: > + if (handler->mh_write == NULL) { > + rc = MGMT_ERR_ENOENT; > } else { > - goto bad_req; > + rc = handler->mh_write(&o->os_cbuf.ob_mj); > } > + break; > + > + default: > + rc = MGMT_ERR_EINVAL; > + break; > } > - if (rc) { > - goto bad_req; > + if (rc != 0) { > + goto done; > + } > + > + /* Encode the NMP header in the response. */ > + rc = omgr_encode_nmp_hdr(&o->os_cbuf.ob_mj.encoder, hdr); > + if (rc != 0) { > + rc = MGMT_ERR_ENOMEM; > + goto done; > } > break; > + > default: > break; > } > > - cbor_encoder_close_container(&g_encoder, &o->os_cbuf.ob_mj.encoder); > - oc_send_response(req, OC_STATUS_OK); > + rc = 0; > > - return; > -bad_req: > - /* > - * XXXX might send partially constructed response as payload > - */ > - if (rc == MGMT_ERR_ENOMEM) { > - rc = OC_STATUS_INTERNAL_SERVER_ERROR; > - } else { > - rc = OC_STATUS_BAD_REQUEST; > - } > - oc_send_response(req, rc); > -} > +done: > + if (rc != 0) { > + if (rsp_hdr_filled) { > + rc = omgr_send_err_rsp(&g_encoder, &hdr, rc); > + } > + switch (rc) { > + case 0: > + break; > > -static void > -omgr_oic_get(oc_request_t *req, oc_interface_mask_t mask) > -{ > - omgr_oic_op(req, mask, 0); > -} > + case MGMT_ERR_ENOMEM: > + rc = OC_STATUS_INTERNAL_SERVER_ERROR; > + break; > > -static void > -omgr_oic_put(oc_request_t *req, oc_interface_mask_t mask) > -{ > - omgr_oic_op(req, mask, 1); > + default: > + rc = OC_STATUS_BAD_REQUEST; > + break; > + } > + } > + > + cbor_encoder_close_container(&g_encoder, &o->os_cbuf.ob_mj.encoder); > + oc_send_response(req, rc); > } > > static void > @@ -204,7 +277,6 @@ omgr_event_start(struct os_event *ev) > oc_resource_bind_resource_interface(res, mode); > oc_resource_set_default_interface(res, mode); > oc_resource_set_discoverable(res); > - oc_resource_set_request_handler(res, OC_GET, omgr_oic_get); > oc_resource_set_request_handler(res, OC_PUT, omgr_oic_put); > oc_add_resource(res); > } >
