Skip to content

!warnings:ARM compiler - fixed code vs inconsisting with standart C, … #44

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

!warnings:ARM compiler - fixed code vs inconsisting with standart C, … #44

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 13 additions & 14 deletions lib/usbd/usbd_ep0.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,21 +177,21 @@ static int utf8_to_utf16(const uint8_t *utf8, uint16_t *utf16,
/* Reference: https://en.wikipedia.org/wiki/UTF-16 */

/* utf-8? */
if (utf32_ch & 0b10000000) {
if ((utf32_ch & 0x80) != 0) {
/* Decoding header */
uint8_t trailing_bytes_utf8;
if ((utf32_ch & 0b11100000) == 0b11000000) {
if ((utf32_ch & 0xe0) == 0xc0) {
/* 110xxxxx 10xxxxxx */
trailing_bytes_utf8 = 1;
utf32_ch &= 0b00011111;
} else if ((utf32_ch & 0b11110000) == 0b11100000) {
utf32_ch &= 0x1f;
} else if ((utf32_ch & 0xf0) == 0xe0) {
/* 1110xxxx 10xxxxxx 10xxxxxx */
trailing_bytes_utf8 = 2;
utf32_ch &= 0b00001111;
} else if ((utf32_ch & 0b11111000) == 0b11110000) {
utf32_ch &= 0x0f;
} else if ((utf32_ch & 0xf8) == 0xf0) {
/* 11110xxx 10xxxxxx 10xxxxxx 10xxxxxx */
trailing_bytes_utf8 = 3;
utf32_ch &= 0b00000111;
utf32_ch &= 0x07;
} else {
/* 111110xx 10xxxxxx 10xxxxxx 10xxxxxx 10xxxxxx */
/* 1111110x 10xxxxxx 10xxxxxx 10xxxxxx 10xxxxxx 10xxxxxx */
Expand All @@ -208,7 +208,7 @@ static int utf8_to_utf16(const uint8_t *utf8, uint16_t *utf16,

/* trailing bytes payload */
while (trailing_bytes_utf8-- > 0) {
utf32_ch = (utf32_ch << 6) | (utf8[i++] & 0b00111111);
utf32_ch = (utf32_ch << 6) | (utf8[i++] & 0x3f);
}
Copy link
Contributor

@kuldeepdhaka kuldeepdhaka Dec 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there was a specific reason (readability imo) to write these specific constant in binary notation.
but hexadecimal notation wont do any improvement either.
Update: (0b binary notation) dont seems to be in C specs, your point is valid.


/* UTF-8 was restricted by RFC 3629 to end at U+10FFFF, in order
Expand Down Expand Up @@ -340,13 +340,13 @@ standard_get_descriptor_string(usbd_device *dev,

if (!index) {
/* Build the list of available langauge strings */
used = build_available_lang(dev, sd->wData, max_count);
used = build_available_lang(dev, (uint16_t*)sd->wData, max_count);
} else {
/* Search for the UTF-8 string and convert it to UTF-16 */
const uint8_t *utf8_str;
utf8_str = search_utf8_string(dev, arg->setup->wIndex, index - 1);
used = (utf8_str == NULL) ? -1 :
utf8_to_utf16(utf8_str, sd->wData, max_count);
utf8_to_utf16(utf8_str, (uint16_t*)sd->wData, max_count);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/insane-adding-machines/unicore-mx/blob/master/include/unicore-mx/usb/usbstd.h#L257
sd->wData is already uint16_t

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that is not: sd->wData is of type "packed" uint16_t*, since it got from packed struct.
maybe there should be some comments. But , i guess, the temporary buffer preserve.buf - always aligned, so better to dropoff "packed" attribute, instead rewrite utf8_to_utf16 and search_utf8_string capable work with unaligned utf16 strings.

}

if (used < 0) {
Expand Down Expand Up @@ -833,9 +833,7 @@ standard_request(usbd_device *dev, struct usbd_control_arg *arg)
void usbd_ep0_setup(usbd_device *dev, const struct usb_setup_data *setup_data)
{
/* Only handle Standard request. */
if ((setup_data->bmRequestType & USB_REQ_TYPE_TYPE) != USB_REQ_TYPE_STANDARD) {
goto stall;
}
if ((setup_data->bmRequestType & USB_REQ_TYPE_TYPE) == USB_REQ_TYPE_STANDARD) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code style: 👎 from me.


struct usbd_control_arg arg = {
.setup = setup_data,
Expand All @@ -849,8 +847,9 @@ void usbd_ep0_setup(usbd_device *dev, const struct usb_setup_data *setup_data)
arg.complete);
return;
}
}//if ( ...== USB_REQ_TYPE_STANDARD)

stall:
//stall:
usbd_ep0_stall(dev);
}

Expand Down
9 changes: 6 additions & 3 deletions lib/usbd/usbd_private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,8 @@ inline void mark_ep_as_free(usbd_device *dev, uint8_t ep_addr, bool yes);
* @param[in] ep_addr Endpoint address (including direction)
* @return mask
*/
inline uint32_t ep_free_mask(uint8_t ep_addr)
static inline
uint32_t ep_free_mask(uint8_t ep_addr)
{
uint32_t num = ENDPOINT_NUMBER(ep_addr);

Expand Down Expand Up @@ -408,7 +409,8 @@ inline void usbd_handle_reset(usbd_device *dev)
* @param[in] dev USB Device
* @param[in] ep_addr Endpoint (including direction)
*/
inline bool is_ep_free(usbd_device *dev, uint8_t ep_addr)
static inline
bool is_ep_free(usbd_device *dev, uint8_t ep_addr)
{
return !!(dev->urbs.ep_free & ep_free_mask(ep_addr));
}
Expand All @@ -419,7 +421,8 @@ inline bool is_ep_free(usbd_device *dev, uint8_t ep_addr)
* @param[in] ep_addr Endpoint address (including direction)
* @param[in] yes Yes (if true, mark it as unused)
*/
inline void mark_ep_as_free(usbd_device *dev, uint8_t ep_addr, bool yes)
static inline
void mark_ep_as_free(usbd_device *dev, uint8_t ep_addr, bool yes)
{
uint32_t mask = ep_free_mask(ep_addr);
if (yes) {
Expand Down
6 changes: 3 additions & 3 deletions lib/usbd/usbd_transfer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,7 +670,7 @@ static void _control_status_callback(usbd_device *dev,
return;
}

usbd_control_transfer_callback callback = transfer->user_data;
usbd_control_transfer_callback callback = (usbd_control_transfer_callback)transfer->user_data;
if (callback != NULL) {
callback(dev, NULL);
}
Expand Down Expand Up @@ -720,7 +720,7 @@ static void _control_data_callback(usbd_device *dev,
return;
}

usbd_control_transfer_callback callback = transfer->user_data;
usbd_control_transfer_callback callback = (usbd_control_transfer_callback)transfer->user_data;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C standard says: void * will automatically cast to any other type.
and this style is more maintanable.
see: http://stackoverflow.com/a/605858/1500988
note to myself: place link to C standard text.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But with some stricts - you can assign any pointer TO void* ,

but if you want assign some* to typed* - need casts to destination type

usbd_control_transfer_feedback feedback = USBD_CONTROL_TRANSFER_OK;

if (callback != NULL) {
Expand Down Expand Up @@ -897,7 +897,7 @@ void *usbd_urb_get_buffer_pointer(usbd_device *dev, usbd_urb *urb, size_t len)
return transfer->buffer;
}

return transfer->buffer + transfer->transferred;
return (void*)((uintptr_t)transfer->buffer + transfer->transferred);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isnt transfer->buffer + transfer->transferred pointer arithmetic (valid in c).
what is the reason of writing it in a more obsured way?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

void* have no information about what entry size is. So yes - compiler is right, when error on such a trick.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You point is valid.
GCC treat void * pointer same as char *, but that is non-standard behaviour.

}

/**
Expand Down