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

Raspberry usb fix #92

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

JLMorange
Copy link

this is a bugfixes to allow to tune low level usb parameter in case of usb instabilities

@nxpfrankli
Copy link
Contributor

please fix build error

@JLMorange
Copy link
Author

I would gladely... if I had some windows build environment.... I tried to find the way you export symbols on windows (usually a EXPORT macro is used... this is not the case... )
so I'm puzzled and can't figure how to fix this windows build issue...

@nxpfrankli
Copy link
Contributor

you may missed conf.cpp in visual studio's project file.
you can use appveryor to check you result.

Copy link
Contributor

@nxpfrankli nxpfrankli left a comment

Choose a reason for hiding this comment

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

==

if (str.size() > 2 &&
str.substr(0, 2).compare("0x") == 0)
{
val = strtoul(start+2, &endPtr, 16);
ss << std::hex << str.substr(2);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

what's problem do you try to fix? the function should be the same

Copy link
Author

Choose a reason for hiding this comment

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

don't know why but the old code fail to convert on x86_64 latest ubuntu.... and well c++ conversion function are more robust than the strtoul legacy

Copy link
Contributor

Choose a reason for hiding this comment

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

It pass ubuntu 18.08 at auto build system.

string size = get_next_param(m_cmd, pos);
Conf::GetConf().m_USB.m_HIDMaxTransfer = str_to_uint(size);
cout << "changed USB::HID::MAX_PACKET_SIZE to " << Conf::GetConf().m_USB.m_HIDMaxTransfer << endl;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid print at libuuu, which will block UI deplay.
Please use notification function to tell uplayer application. Show or hide this information decide by application.

set_last_err_string(err);
return -1;

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Dose CmdBase::insert_param_info match your parser requirement? That simplify parser work.

@@ -289,6 +298,7 @@ CmdObjCreateMap::CmdObjCreateMap()

(*this)["_ALL:DONE"] = new_cmd_obj<CmdDone>;
(*this)["_ALL:DELAY"] = new_cmd_obj<CmdDelay>;
(*this)["_ALL:CONFIG"] = new_cmd_obj<CmdConfig>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure all protocol need config command.

There are already have CFG: protocol to set PID VID to protocol map?

which will run once at beginning.

CFG: HID -maxtimeout 1000

Copy link
Author

Choose a reason for hiding this comment

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

what is CFG protocol? this isn't documented... I don't understand your remark....

Copy link
Contributor

Choose a reason for hiding this comment

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

in help

 CFG:  Config protocol of specific usb device vid/pid
            SDPS|SDP|FB\Fastboot|FBK -chip <chip name> -pid <pid> -vid <vid  

@@ -310,11 +314,13 @@ int FBCopy::run(CmdCtx *ctx)
nt.total = buff->size();
call_notify(nt);

for (i = 0; i < buff->size(); i += this->m_Maxsize_pre_cmd)
for (i = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

m_Maxsize_pre_cmd need be update at beginning, such as construct function or at parser function. so needn't change below code.

Copy link
Author

Choose a reason for hiding this comment

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

the code is needed here because we have to know about the packet size to split the content, using construct fonction or at parser level will imply lot of modifications since there is no way to go from parser to low level constructor, doing so will require modify lot of API... I didn't wanted to go that way because it overcomplicate the process, while a configuration class singleton store the configuration in a dedicated place and is easely integrated

sz,
&actual,
Conf::GetConf().m_USB.m_HIDTimeout
);
Copy link
Contributor

Choose a reason for hiding this comment

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

you should update m_read_timeout.

Copy link
Author

Choose a reason for hiding this comment

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

read time out seems to be something not tunable if I correctly remember.... only write timeout is meaningfull for tunning (remembered seeing that in libusb documentation...)

<< Conf::GetConf().m_USB.m_BulkTimeout
<< " ms"
<< endl;
retries_on_timeout = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

All error message should use set_last_error_string. Or notification function.

@nxpfrankli nxpfrankli force-pushed the master branch 5 times, most recently from 1597a12 to 0f8362f Compare June 6, 2023 22:48
@nxpfrankli nxpfrankli force-pushed the master branch 10 times, most recently from 629d224 to 8925b6c Compare June 15, 2023 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants