Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1316543496-18575-1-git-send-email-lucas.demarchi@profusion.mobi> <1316543496-18575-3-git-send-email-lucas.demarchi@profusion.mobi> From: Lucas De Marchi Date: Wed, 21 Sep 2011 09:52:37 -0300 Message-ID: Subject: Re: [PATCH 2/3] avrcp: implement pdu continuation request and abort To: Luiz Augusto von Dentz Cc: linux-bluetooth@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Luis On Wed, Sep 21, 2011 at 7:14 AM, Luiz Augusto von Dentz wrote: > Hi Lucas, > > On Tue, Sep 20, 2011 at 9:31 PM, Lucas De Marchi > wrote: >> GetElementAttributes is the only one that can possibly overflow the MTU >> of AVC. When this command is received, if the response is larger than >> the MTU split the messages and queue them for later processing. >> --- >> ?audio/avrcp.c | ?282 ++++++++++++++++++++++++++++++++++++++++++++++----------- >> ?1 files changed, 229 insertions(+), 53 deletions(-) >> >> diff --git a/audio/avrcp.c b/audio/avrcp.c >> index ba04a12..8fccede 100644 >> --- a/audio/avrcp.c >> +++ b/audio/avrcp.c >> @@ -64,6 +64,12 @@ >> ?#define E_PARAM_NOT_FOUND ? ? ?0x02 >> ?#define E_INTERNAL ? ? ? ? ? ? 0x03 >> >> +/* Packet types */ >> +#define AVRCP_PACKET_TYPE_SINGLE ? ? ? 0x00 >> +#define AVRCP_PACKET_TYPE_START ? ? ? ? ? ? ? ?0x01 >> +#define AVRCP_PACKET_TYPE_CONTINUING ? 0x02 >> +#define AVRCP_PACKET_TYPE_END ? ? ? ? ?0x03 >> + >> ?/* PDU types for metadata transfer */ >> ?#define AVRCP_GET_CAPABILITIES ? ? ? ? 0x10 >> ?#define AVRCP_LIST_PLAYER_ATTRIBUTES ? 0X11 >> @@ -77,6 +83,8 @@ >> ?#define AVRCP_GET_ELEMENT_ATTRIBUTES ? 0x20 >> ?#define AVRCP_GET_PLAY_STATUS ? ? ? ? ?0x30 >> ?#define AVRCP_REGISTER_NOTIFICATION ? ?0x31 >> +#define AVRCP_REQUEST_CONTINUING ? ? ? 0x40 >> +#define AVRCP_ABORT_CONTINUING ? ? ? ? 0x41 >> >> ?/* Notification events */ >> ?#define AVRCP_EVENT_PLAYBACK_STATUS_CHANGED ? ? ? ? ? ?0x01 >> @@ -201,6 +209,7 @@ struct media_player { >> >> ? ? ? ?struct media_info mi; >> ? ? ? ?GTimer *timer; >> + ? ? ? GQueue *pending_pdus; >> ? ? ? ?unsigned int handler; >> ? ? ? ?uint16_t registered_events; >> ? ? ? ?uint8_t transaction_events[AVRCP_EVENT_TRACK_CHANGED + 1]; >> @@ -645,15 +654,17 @@ static void mp_set_playback_status(struct media_player *mp, uint8_t status, >> ?* Copy media_info field to a buffer, intended to be used in a response to >> ?* GetElementAttributes message. >> ?* >> - * It assumes there's enough space in the buffer and on success it returns the >> - * size written. >> + * If there isn't enough space in the buffer, the available space is filled, >> + * the remainder (both header and value) is allocated in @param remainder and >> + * its size is written to remainder_len. >> ?* >> - * If @param id is not valid, -EINVAL is returned. If there's no such media >> - * attribute, -ENOENT is returned. >> + * On success, it returns the size written. Otherwise it returns -EINVAL if >> + * @param id is not valid or -ENOENT if there's no such media attribute. >> ?*/ >> ?static int mp_get_media_attribute(struct media_player *mp, >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? uint32_t id, uint8_t *buf, >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? uint16_t maxlen) >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? uint32_t id, uint8_t *buf, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? uint16_t maxlen, uint8_t **remainder, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int *remainder_len) >> ?{ >> ? ? ? ?struct media_info_elem { >> ? ? ? ? ? ? ? ?uint32_t id; >> @@ -662,16 +673,16 @@ static int mp_get_media_attribute(struct media_player *mp, >> ? ? ? ? ? ? ? ?uint8_t val[]; >> ? ? ? ?}; >> ? ? ? ?const struct media_info *mi = &mp->mi; >> - ? ? ? struct media_info_elem *elem = (void *)buf; >> + ? ? ? struct media_info_elem *elem; >> ? ? ? ?uint16_t len; >> ? ? ? ?char valstr[20]; >> ? ? ? ?char *valp; >> + ? ? ? int ret; >> >> - ? ? ? if (maxlen < sizeof(struct media_info_elem)) >> - ? ? ? ? ? ? ? return -ENOBUFS; >> + ? ? ? assert(remainder != NULL); >> + ? ? ? assert(remainder_len != NULL); > > I guess you probably assert(remainder_len > 0) since remainder_len is > int, well perhaps size_t fits better here. No... I'm asserting I was not dumb to call this function with remainder_len being a NULL pointer. If *remainder_len == 0 we're better dealing with it instead of aborting the entire process. > >> - ? ? ? /* Subtract the size of elem header from the available space */ >> - ? ? ? maxlen -= sizeof(struct media_info_elem); >> + ? ? ? DBG("id=%u buf=%p maxlen=%u", id, buf, maxlen); >> >> ? ? ? ?switch (id) { >> ? ? ? ?case MEDIA_INFO_TITLE: >> @@ -720,22 +731,50 @@ static int mp_get_media_attribute(struct media_player *mp, >> ? ? ? ? ? ? ? ?return -EINVAL; >> ? ? ? ?} >> >> - ? ? ? if (valp) { >> + ? ? ? if (valp) >> ? ? ? ? ? ? ? ?len = strlen(valp); >> + ? ? ? else >> + ? ? ? ? ? ? ? len = 0; >> + >> + ? ? ? if (maxlen < sizeof(struct media_info_elem)) { >> + ? ? ? ? ? ? ? /* >> + ? ? ? ? ? ? ? ?* There isn't space even for the header: put both header and >> + ? ? ? ? ? ? ? ?* value in remainder. >> + ? ? ? ? ? ? ? ?*/ >> + ? ? ? ? ? ? ? *remainder_len = sizeof(struct media_info_elem) + len; >> + ? ? ? ? ? ? ? elem = g_malloc(*remainder_len); >> + ? ? ? ? ? ? ? *remainder = (uint8_t *) elem; >> >> - ? ? ? ? ? ? ? if (len > maxlen) >> - ? ? ? ? ? ? ? ? ? ? ? return -ENOBUFS; >> + ? ? ? ? ? ? ? if (len) >> + ? ? ? ? ? ? ? ? ? ? ? memcpy(elem->val, valp, len); >> >> - ? ? ? ? ? ? ? memcpy(elem->val, valp, len); >> + ? ? ? ? ? ? ? ret = 0; >> ? ? ? ?} else { >> - ? ? ? ? ? ? ? len = 0; >> + ? ? ? ? ? ? ? /* Put at least header on current PDU */ >> + ? ? ? ? ? ? ? elem = (struct media_info_elem *) buf; >> + ? ? ? ? ? ? ? maxlen -= sizeof(struct media_info_elem); >> + >> + ? ? ? ? ? ? ? if (maxlen < len) { >> + ? ? ? ? ? ? ? ? ? ? ? /* Split value between current PDU and remainder. */ >> + ? ? ? ? ? ? ? ? ? ? ? memcpy(elem->val, valp, maxlen); >> + ? ? ? ? ? ? ? ? ? ? ? *remainder_len = len - maxlen; >> + ? ? ? ? ? ? ? ? ? ? ? *remainder = g_memdup(valp + maxlen, *remainder_len); >> + ? ? ? ? ? ? ? ? ? ? ? ret = sizeof(struct media_info_elem) + maxlen; >> + ? ? ? ? ? ? ? } else { >> + ? ? ? ? ? ? ? ? ? ? ? /* Header and value fit in current PDU */ >> + ? ? ? ? ? ? ? ? ? ? ? if (len) >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? memcpy(elem->val, valp, len); >> + >> + ? ? ? ? ? ? ? ? ? ? ? *remainder = NULL; >> + ? ? ? ? ? ? ? ? ? ? ? ret = sizeof(struct media_info_elem) + len; >> + ? ? ? ? ? ? ? } >> ? ? ? ?} > > memcpy, hmm bad sign. Notice that I removed a memcpy, too. More on this below. > >> ? ? ? ?elem->id = htonl(id); >> ? ? ? ?elem->charset = htons(0x6A); /* Always use UTF-8 */ >> ? ? ? ?elem->len = htons(len); >> >> - ? ? ? return sizeof(struct media_info_elem) + len; >> + ? ? ? return ret; >> ?} >> >> ?static void mp_set_attribute(struct media_player *mp, >> @@ -890,64 +929,116 @@ err: >> ? ? ? ?return AVC_CTYPE_REJECTED; >> ?} >> >> +static struct avrcp_header *avrcp_split_pdu(struct media_player *mp, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct avrcp_header *last_pdu, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? uint8_t *remainder, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int remainder_len, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? uint16_t *pos) >> +{ >> + ? ? ? struct avrcp_header *pdu = last_pdu; >> + ? ? ? uint16_t len; >> + >> + ? ? ? assert(remainder != NULL); >> + ? ? ? assert(pos != NULL); >> + >> + ? ? ? if (!mp->pending_pdus) >> + ? ? ? ? ? ? ? mp->pending_pdus = g_queue_new(); >> + >> + ? ? ? for (len = *pos; remainder_len; remainder_len -= len) { >> + ? ? ? ? ? ? ? /* Close last pdu - keep host endiannes */ >> + ? ? ? ? ? ? ? pdu->params_len = len; >> + >> + ? ? ? ? ? ? ? /* Create a new PDU */ >> + ? ? ? ? ? ? ? pdu = g_malloc(AVRCP_MTU); >> + >> + ? ? ? ? ? ? ? memcpy(pdu, last_pdu, sizeof(struct avrcp_header)); >> + ? ? ? ? ? ? ? g_queue_push_tail(mp->pending_pdus, pdu); >> + >> + ? ? ? ? ? ? ? if (remainder_len > AVRCP_PDU_MTU) >> + ? ? ? ? ? ? ? ? ? ? ? len = AVRCP_PDU_MTU; >> + ? ? ? ? ? ? ? else >> + ? ? ? ? ? ? ? ? ? ? ? len = remainder_len; >> + >> + ? ? ? ? ? ? ? memcpy(&pdu->params[0], remainder, len); >> + ? ? ? } > > Second memcpy, this does not look good. > >> + >> + ? ? ? *pos = len; >> + >> + ? ? ? return pdu; >> +} >> + >> ?static uint8_t avrcp_handle_get_element_attributes(struct media_player *mp, >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct avrcp_header *pdu, >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? uint8_t transaction) >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct avrcp_header *first_pdu, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? uint8_t transaction) >> ?{ >> - ? ? ? uint16_t len = ntohs(pdu->params_len); >> + ? ? ? struct avrcp_header *pdu = first_pdu; >> ? ? ? ?uint64_t *identifier = (void *) &pdu->params[0]; >> + ? ? ? uint32_t attr_ids[MEDIA_INFO_LAST]; >> + ? ? ? uint16_t len = ntohs(pdu->params_len); >> ? ? ? ?uint16_t pos; >> ? ? ? ?uint8_t nattr; >> - ? ? ? int size; >> ? ? ? ?unsigned int i; >> >> ? ? ? ?if (len < 8 || *identifier != 0) >> ? ? ? ? ? ? ? ?goto err; >> >> - ? ? ? len = 0; >> - ? ? ? pos = 1; /* Keep track of current position in reponse */ >> ? ? ? ?nattr = pdu->params[8]; >> >> - ? ? ? if (!nattr) { >> - ? ? ? ? ? ? ? /* >> - ? ? ? ? ? ? ? ?* Return all available information, at least >> - ? ? ? ? ? ? ? ?* title must be returned. >> - ? ? ? ? ? ? ? ?*/ >> - ? ? ? ? ? ? ? for (i = 1; i < MEDIA_INFO_LAST; i++) { >> - ? ? ? ? ? ? ? ? ? ? ? size = mp_get_media_attribute(mp, i, &pdu->params[pos], >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? AVRCP_PDU_MTU - pos); >> - >> - ? ? ? ? ? ? ? ? ? ? ? if (size > 0) { >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? len++; >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pos += size; >> - ? ? ? ? ? ? ? ? ? ? ? } >> + ? ? ? DBG("nattr %u", nattr); >> + >> + ? ? ? /* Copy the requested attribute ids to our private vector */ >> + ? ? ? if (nattr) { >> + ? ? ? ? ? ? ? uint32_t *attr = (uint32_t *) &pdu->params[9]; >> + ? ? ? ? ? ? ? if (nattr > MEDIA_INFO_LAST) { >> + ? ? ? ? ? ? ? ? ? ? ? error("Got number of attributes %u > %u", >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? nattr, MEDIA_INFO_LAST); >> + ? ? ? ? ? ? ? ? ? ? ? nattr = MEDIA_INFO_LAST; >> ? ? ? ? ? ? ? ?} >> + >> + ? ? ? ? ? ? ? for (i = 0; attr < (uint32_t *)&pdu->params[9] + >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? nattr * sizeof(uint32_t); >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? attr++, i++) >> + ? ? ? ? ? ? ? ? ? ? ? attr_ids[i] = ntohl(*attr); >> ? ? ? ?} else { >> - ? ? ? ? ? ? ? uint32_t *attr_ids; >> + ? ? ? ? ? ? ? nattr = MEDIA_INFO_LAST - 1; >> >> - ? ? ? ? ? ? ? attr_ids = g_memdup(&pdu->params[9], sizeof(uint32_t) * nattr); >> + ? ? ? ? ? ? ? for (i = 1; i < MEDIA_INFO_LAST; i++) >> + ? ? ? ? ? ? ? ? ? ? ? attr_ids[i - 1] = i; >> + ? ? ? } >> >> - ? ? ? ? ? ? ? for (i = 0; i < nattr; i++) { >> - ? ? ? ? ? ? ? ? ? ? ? uint32_t attr = ntohl(attr_ids[i]); >> + ? ? ? for (i = 0, len = 0, pos = 1; i < nattr; i++) { >> + ? ? ? ? ? ? ? uint8_t *remainder; >> + ? ? ? ? ? ? ? int size, remainder_len; >> >> - ? ? ? ? ? ? ? ? ? ? ? size = mp_get_media_attribute(mp, attr, >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &pdu->params[pos], >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? AVRCP_PDU_MTU - pos); >> + ? ? ? ? ? ? ? size = mp_get_media_attribute(mp, attr_ids[i], >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &pdu->params[pos], >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? AVRCP_PDU_MTU - pos, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &remainder, &remainder_len); >> >> - ? ? ? ? ? ? ? ? ? ? ? if (size > 0) { >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? len++; >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pos += size; >> - ? ? ? ? ? ? ? ? ? ? ? } >> - ? ? ? ? ? ? ? } >> + ? ? ? ? ? ? ? if (size < 0) >> + ? ? ? ? ? ? ? ? ? ? ? continue; >> >> - ? ? ? ? ? ? ? g_free(attr_ids); >> + ? ? ? ? ? ? ? pos += size; >> + ? ? ? ? ? ? ? len++; >> >> - ? ? ? ? ? ? ? if (!len) >> - ? ? ? ? ? ? ? ? ? ? ? goto err; >> + ? ? ? ? ? ? ? if (remainder) { >> + ? ? ? ? ? ? ? ? ? ? ? pdu = avrcp_split_pdu(mp, pdu, remainder, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? remainder_len, &pos); >> + ? ? ? ? ? ? ? ? ? ? ? g_free(remainder); >> + ? ? ? ? ? ? ? } >> ? ? ? ?} >> >> - ? ? ? pdu->params[0] = len; >> - ? ? ? pdu->params_len = htons(pos); >> + ? ? ? if (!len) >> + ? ? ? ? ? ? ? goto err; >> + >> + ? ? ? /* Close last pdu - keep host endiannes */ >> + ? ? ? pdu->params_len = pos; >> + >> + ? ? ? if (first_pdu != pdu) >> + ? ? ? ? ? ? ? first_pdu->packet_type = AVRCP_PACKET_TYPE_START; >> + >> + ? ? ? first_pdu->params[0] = len; >> + ? ? ? first_pdu->params_len = htons(first_pdu->params_len); >> >> ? ? ? ?return AVC_CTYPE_STABLE; >> ?err: >> @@ -1192,6 +1283,82 @@ err: >> ? ? ? ?return AVC_CTYPE_REJECTED; >> ?} >> >> +static void cancel_pending_pdus(struct media_player *mp) >> +{ >> + ? ? ? struct avrcp_header *pdu; >> + >> + ? ? ? if (!mp || !mp->pending_pdus) >> + ? ? ? ? ? ? ? return; >> + >> + ? ? ? DBG("%u PDUs cancelled.", g_queue_get_length(mp->pending_pdus)); >> + >> + ? ? ? while ((pdu = g_queue_pop_head(mp->pending_pdus))) >> + ? ? ? ? ? ? ? g_free(pdu); >> + >> + ? ? ? g_queue_free(mp->pending_pdus); >> + ? ? ? mp->pending_pdus = NULL; >> +} >> + >> +static uint8_t avrcp_handle_request_continuing(struct media_player *mp, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct avrcp_header *pdu, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? uint8_t transaction) >> +{ >> + ? ? ? struct avrcp_header *saved_pdu; >> + ? ? ? int len; >> + >> + ? ? ? if (!mp || !mp->pending_pdus) { >> + ? ? ? ? ? ? ? pdu->params_len = htons(1); >> + ? ? ? ? ? ? ? pdu->params[0] = E_INVALID_PARAM; >> + ? ? ? ? ? ? ? return AVC_CTYPE_REJECTED; >> + ? ? ? } >> + >> + ? ? ? DBG(""); >> + >> + ? ? ? saved_pdu = g_queue_pop_head(mp->pending_pdus); >> + >> + ? ? ? len = saved_pdu->params_len; >> + ? ? ? memcpy(pdu, saved_pdu, sizeof(struct avrcp_header) + len); >> + ? ? ? pdu->params_len = htons(len); > > Third memcpy, I guess we need to fix this somehow. > >> + ? ? ? g_free(saved_pdu); >> + >> + ? ? ? if (g_queue_is_empty(mp->pending_pdus)) { >> + ? ? ? ? ? ? ? pdu->packet_type = AVRCP_PACKET_TYPE_END; >> + ? ? ? ? ? ? ? g_queue_free(mp->pending_pdus); >> + ? ? ? ? ? ? ? mp->pending_pdus = NULL; >> + ? ? ? } else { >> + ? ? ? ? ? ? ? pdu->packet_type = AVRCP_PACKET_TYPE_CONTINUING; >> + ? ? ? } >> + >> + ? ? ? return AVC_CTYPE_STABLE; >> +} >> + >> +static uint8_t avrcp_handle_abort_continuing(struct media_player *mp, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct avrcp_header *pdu, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? uint8_t transaction) >> +{ >> + ? ? ? uint16_t len = ntohs(pdu->params_len); >> + ? ? ? struct avrcp_header *saved_pdu; >> + >> + ? ? ? if (len < 1 || mp->pending_pdus == NULL) >> + ? ? ? ? ? ? ? goto err; >> + >> + ? ? ? saved_pdu = g_queue_peek_head(mp->pending_pdus); >> + >> + ? ? ? if (saved_pdu->pdu_id != pdu->params[0]) >> + ? ? ? ? ? ? ? goto err; > > There is something missing here, don't you have to free the queue after abort? Yes, I forgot it when rebased. I'll fix that. > I wonder if you have tried a more simplistic approach without parsing > and storing all the pdus in a queue since the beginning, we could > store just store the list of pending attributes to be sent and > generate the pdu only when requested/needed, that would probably > eliminate most of the memcpy. Yes, but it's not so simple as it looks at first: 1) The attributes may split in the middle. So we would need to store the attribute, and the position within it that might be already sent. Actually splitting on attribute boundaries will happen only in the rare case where the avrcp header doesn't fit in the current pdu. Heck, the attribute can even span to more than 2 PDUs. 2) We would need to save the list of requested attributes too, since it's not true that we're always responding with all attributes of the track. This would mean to parse the requested attributes as we are doing now, but also validating (CT might ask for invalid attributes) and storing them somewhere. 3) The attributes may change while responding to RequestContinuing commands. What if the user called ChangeTrack() before we finish the pending requests? Since (i) we can't simply continue, (ii) the CT might still want to know the track that was playing and (iii) it's CT's role to abort the previous request, the only solution that I see would be to save the media_info struct too. While implementing this I considered this RequestContinuing/AbortContinuing will have very few usage since 512 bytes is probably enough most of the time (this may not be true if we consider we are transferring UTF-8 strings that might span more bytes per character). So we only start allocating space (and memcpy'ing) if we reach this situation. We could remove 1 memcpy if we share the header among the PDUs but it didn't feel appealing for such a small amount of bytes 'cause it would complicate things when continuing. thanks Lucas De Marchi