Skip to content
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

Avr port #82

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

Avr port #82

wants to merge 16 commits into from

Conversation

perigoso
Copy link
Member

@perigoso perigoso commented Jul 1, 2021

closes #57

cc @ljmf00

@perigoso perigoso added the porting Porting to new family or target label Jul 1, 2021
@perigoso perigoso self-assigned this Jul 1, 2021
@perigoso perigoso mentioned this pull request Jan 25, 2022
3 tasks
@perigoso perigoso force-pushed the avr-port branch 2 times, most recently from 0c6a24e to dbe07f8 Compare April 15, 2022 16:25
@perigoso
Copy link
Member Author

TODO before review:

  • protocol handling

Copy link

@ljmf00 ljmf00 left a comment

Choose a reason for hiding this comment

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

I'm aware this is in draft, although took a quick look and decide to leave some comments :) I don't have an in-depth knowledge of the project so this was rather superficial.


void usb_write_descriptor(u8 interface, u8 *report_data, u16 report_size)
{
if (interface == openinput_hid_interface.Config.InterfaceNumber) {
Copy link

Choose a reason for hiding this comment

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

I would use a switch case here.

};

/* create protocol config */
struct protocol_config_t protocol_config;
Copy link

Choose a reason for hiding this comment

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

I don't know what C version do openinput use, but this is possible on newer versions for zero initialization:

Suggested change
struct protocol_config_t protocol_config;
struct protocol_config_t protocol_config = {};

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we're freezing to a particular standard right now, but we do or will, it would be one that supports this

Either way, static uninitialized variables end up on the .bss and are set to 0 by default, at least on our arm targets

* passed to all HID Class driver functions, so that multiple instances of the same class
* within a device can be differentiated from one another.
*/
USB_ClassInfo_HID_Device_t openinput_hid_interface = {
Copy link

Choose a reason for hiding this comment

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

Is this aliased to const? I don't see changes to this memory so perhaps this can be on .data section or other read-only memory.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just me playing with lufa and committing to save progress, this is will get a clean up

Copy link
Member Author

Choose a reason for hiding this comment

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

The struct itself i don't think is written, so yes, although on avrs, consts are always fetched to ram anyway i believe

Copy link

Choose a reason for hiding this comment

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

The struct itself i don't think is written, so yes, although on avrs, consts are always fetched to ram anyway i believe

Well, yeah, I'm not aware of those embedded cases. Godbolt https://godbolt.org/z/soo4eaaev tells me it goes to .rodata and the compiler can optimize it to immediate loads instead of an offset. This is not always the case although, as I believe the jump cost is a trade-off the compiler takes into consideration, depending on size of the data/non-natural alignment.

perigoso added 16 commits June 9, 2022 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
porting Porting to new family or target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Atmega32U4 support
2 participants