-
-
Notifications
You must be signed in to change notification settings - Fork 725
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
USB PD timeout added as an option for improved QC3 compatibility #994
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hia,
Mostly changes in how you have adjusted the PD class, the state machine class is not to import the settings struct, only the user file can't include settings. Also please don't just hide the of state, ensure you are changing the state of the PD machine so that all comms are stopped correctly.
This is not because it doesnt work but instead because the user class expansion file is the place for things that are implementation specific. Making sure the state machine is halted is to ensure that PD it's stopped and never continues such that debugging is black and white for what is going on.
😘
return false; | ||
} | ||
|
||
if (xTaskGetTickCount() > (TICKS_SECOND/10 * systemSettings.PDNegTimeout)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a standalone class, you can't rely on general tick counter, please reference this based on timing out during the wait for capabilities state in the state machine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to make the timeout for the waiting for capabilities state dynamic, don't just patch over it or else pd negotiation may still continue in the background.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to make the timeout for the waiting for capabilities state dynamic, don't just patch over it or else pd negotiation may still continue in the background.
I did it because I have thought if it continues during the background and is successful, then the result of PD negotiation would be much better than QC itself.
Is it a bad idea?
Maybe if QC3 pulses has started then it drops ongoing PD initialization, I don't know.
That case, so if PD negotiation is slow and QC3 pulses break remaining part of negotiation, then it should definitively go to the state machine, but is that the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is indeed the case on a charger and a dock I have to hand, where as soon as you start to mess with the data pins they lock the port to 5v.
Also it's just for clarity sake when debugging a user having an issue, far easier of it's known "once QC starts, PD stops"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a standalone class, you can't rely on general tick counter, please reference this based on timing out during the wait for capabilities state in the state machine
I am afraid I don't know how to do it in a proper way, I thought the way PPSTimerCallback() uses may be good.
One thing is it should put state machine to appropriate state, but another one is I would like to wait for given timeout, regardless of what state the PD negotiation is.
Also, I am not familiar with RTOS yet, so I don't know if I set the state of machine elsewhere, then it may also change the state machine in one of the states (still pending) or... how should I do it.
return false; | ||
} | ||
|
||
if (xTaskGetTickCount() > (TICKS_SECOND/10 * systemSettings.PDNegTimeout)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will also need to take into consideration the 1-2 second bootloader time of the TS80P most likely if you need hard time since boot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be addressed by the setting itself, so if it was set to 1 and bootloader took 1.2 seconds to finish, then it will wait for 1.3 s just like if it was set to 13 without additional wait for bootloader.
If the extra 100 ms wait matters, it may be initialized to -1 for disabled state (and modify variable to signed int) or a higher (special) value, so that way it could be solved for the best possible way of practically instant action (so when main software starts).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if it matters at all, was just noting it might be something to take into awareness, since the "0" mark of xTaskGetTickCount() is once the bootloader finishes, hardware init finishes and we kick off the RToS.
Though I dont think it will matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean it may check against a more native tick counter or timer other than the task's counter?
I don't know if it has a built-in independent counter anyway or a free timer which could be started.
...or you mean GUIThread is started after POWThread and if a call happens from the latter to the former, it may run to an uninitialized variable as task's tick counter?
POWThread calls vTaskDelay(TICK_100MS) prior running power_check(), and GUIThread does not have such a delay and it should finish initialization in 100 ms I think.
Also, timeout's minimal value is currently 1, which also adds an additional 100 ms delay, so I don't think it should be a problem if that delay remains there...
I think OLED's initialization over I2C may be the slowest part of initialization, but as far as I can see, display comes up quickly at least on Pinecil.
Edit: Sorry, additional 100 ms delay is not applied as thread has started at that point.
I thought of doing that way, sorry for did it in that class. I still in doubt if there is any circumstances where this timeout fires but PD negotiation still can be finished using better power capabilities. It would be good to know if that is a valid option. |
CI had some complaints which I hope now should be resolved. |
I think it should go through formatting check now, but I can't test it locally as I think clang-format is a bit older on Debian Stable which I use and it cannot understand well its configuration file (having "Invalid argument" problem). Maybe running it under a recent Ubuntu chroot would solve this but I don't know if it worth the effort. Basically I have used a text-mode editor instead of a correct IDE which would be the usual tool / feature set for normal development. Tomorrow I will be on the roads but I will try to correct if anything comes up as soon as I arrived home late afternoon or early evening. Sorry for left some formatting problems like a bit more spacing or lacking space before opening bracket ({). |
If you are using debian you will generally be a generation or two behind sadly :/ You can use the start_dev command script to spawn a Docker container with everything setup with fairly close versions. Then you can just use the I however, always recommend using an ide such as vscode to ensure you are keeping fairly close to the mark as well. |
Not really, I think based on your photo it should be a maximum of 0.5 generation or most likely around 0.2. :) I use Linux as my desktop since around 2010 and stuck at Debian because it was reliable and long-living even after several major upgrades and it is handy in many ways, but I did not like the always moving orientation of Ubuntu. I did not want to go back to Windows as a daily driver if not necessary, that is in a very good place as virtual machine.
Thank you, I didn't think about it.
Yes, you are right. My mistake was that I thought it is a very minor modification and I was too lazy for doing it the right way. :( Anyway, the last problem was a comment which belongs to the new (PDNegTimeout) variable, but it was moved to the bottom later and its comment was still aligned according to its original place. Sorry, I was not at the top today and tomorrow will be a very exhausting day starting and ending by travel and meetings in-between. |
Huh, last I checked Debian was still on like gcc 9 rather than 11 :) (That may just be their docker images though).
I'm an Arch and Manjaro person 95% of the time so don't worry at all. (Windows for games only). Definitely in agreement there.
No issue at all, sometimes its an easy way to get as close as possible to a 1:1 with github.
100% Respect. Generally agree with you too, have fallen into that trap a lot.
No issue at all, never feel bad. :) |
There are advantages and also disadvantages of a distribution which is generally tries to provide packages as stable as possible. On Debian-like infrastructure, mostly Ubuntu is bleeding-edge which also has its advantages and disadvantages. However, if necessary, GCC is a component which can be easily replaced and it also can be co-existent if needed. There is another point of view when an embedded code works, then if you compile it using a different version of compiler, then the risk of some different behaviour appears is increasing, thus, further testing may be needed before releasing it (side effect, bug in the code or either of compilers, optimization-specific changes, etc). However, that should not really happen, but as there is a risk, it may also be a considerable thing.
I have started with Mandriva (not OpenMandriva but a successor of Mandrake and predecessor of OpenMandriva), which was very handy with KDE. Also, reading texts on it is much better than it was under Windows - however, it may have also improved on Windows as I haven't noticed it recently, but font hinting became much better in those days while it was really ugly on Debian by default without fine-tunings. ...and yes, Windows is definitively very good for gaming.
Generally it is a good idea of keeping the code consistent and every projects have their own coding style which should be respected. So basically it is nothing wrong doing such things.
I am trying to. :) Anyway, it was interesting. ...but I did not see there is an excessive space at the front, however, when I have checked my local repository, the extra space was really not there. So double-checked and went to sleep. Some of the other (untouched) files were changed, mostly containing multi-line comments which were modified to one row and also the row in question which had an excessive space in repository was removed. So I suspect when I run "make style", it also does minor modifications, so I think it has removed the excessive space and that is why it was corrected when I have checked it later, the similar way as it automatically "corrected" multi-line comments in STM32 stock header files. However, I have them reverted as I did not want to touch those files, then pushed the file where the last CI complain was and pushed that modification. So it is interesting that "make style" does automatic modifications of slight styling problems, but fails to run on GitHUB which is also understandable as it should not really touch user files, but was definitively not an expected behaviour of doing automatic modification while it shows everything are perfect. So, it was an interesting experience. |
Wow, all checks were passed now. |
Just generally found there isnt a nice way to run in a mode of "Give me the latest stable of everything" without a lot of screwing around and with things breaking. Butt that is getting us way off topic :)
This is why we pin drivers in this repo :)
Am I weird in that this has not occurred to me at all since I jumped to Windows 10? Just let it reboot at the end of every day and I've never had it interrupt me 🤣
clang-format is not perfectly stable I've found. There is a version somewhere in the good old ubuntu releases that is different to the modern one used everywhere else that can cause grief. I haven't dug into it massively as it hasn't hit me really yet... yet :) Anyway, this looks good by me :) |
Right, sorry for that. :) Anyway, C# would be a very sympathetic language if it was not a mostly Windows-only thing, bat that is also another turn - sorry, I stop now. :)
Oh, sorry, I did not want to "educate" you, just sometimes writing unnecessary things... :)
I think reboot helps a lot. Also, as far as I know it was improved a lot at later builds of Windows 10.
It is not as bad when it does not need an approval of another person ping-ponging of the code validation row by row. :) To be a bit ontopic, I am uncertain if the maximal value of 49 is not too high. ...just because it can be a bit frustrating to go over desired value and keep pushing the button up to 49 and restart then. It is not as important which would definitively need any changes, just asking if it should has any considerations. Anyway, thank you for approving / merging the code, it makes Pinecil much better for me again as there were a lot of improvements since initial revision and it will definitively has further improvements by time, I like it very much. I also feel its PID control / heating is very good, I haven't thought it is as usable as it is for soldering also at lower voltages. So big respect for doing the firmware and also for porting it to Pinecil. |
Adds a PD timeout menu item in power menu which allows to start QC negotiation earlier than the main negotiation process has finished.
Its purpose is to allow some QC3 chargers to work instead of falling back to 5V when QC3 pulses come too lately.
This way PD code was not changed deeply but improves compatibility with some QC chargers as an option.
Currently the timeout value can be set in 100 ms steps up to 5 seconds (from 1 to 50).
Defaults to zero which means disabled and the code should run the same way before this modification.
Tested locally using Pinecil but only on that hardware.
It should not break other functions.