Add check for max number of endpoints #20
Conversation
peter-marcisovsky
force-pushed
the
feature/check_max_endpoint_count
branch
from
0a103cc to
5cbb51a
Compare
peter-marcisovsky
force-pushed
the
feature/check_max_endpoint_count
branch
2 times, most recently
from
4c24ffb to
3c141f2
Compare
There was a problem hiding this comment.
@peter-marcisovsky Thanks a lot! But I was just offered an approach, not suggest anything...
Yes, seems that right now the logic is more clearer, but again, I think that couple of things still could be done better.
One comment about general way.
As always, I am ready to discuss it anytime.
There was a problem hiding this comment.
Hey @peter-marcisovsky,
Sorry, probably I was not so clear in my previous review. I have tried to make more comments and suggestions about my idea and to be more concise.
Important thing: not to forget to decrease the dwc_ep_config[rhport].in_ep or dwc_ep_config[rhport].out_ep during dcd_edpt_close_all and dcd_edpt_close.
peter-marcisovsky
force-pushed
the
feature/check_max_endpoint_count
branch
2 times, most recently
from
30047cb to
6296166
Compare
peter-marcisovsky
force-pushed
the
feature/check_max_endpoint_count
branch
from
6296166 to
6cede67
Compare
There was a problem hiding this comment.
Hey @peter-marcisovsky,
Basically, everything is great! Good job.
Just a couple of suggestion, how to make it better.
I have added several comments from my side.
Regarding those comments, we will have two tasks to complete:
- Eliminate or accept the fact that without debug it will be a silent fail inside the tud_task.
- Think about migrating to OUT_EP_NUM instead of having them here (in two different places actually:
DWC2_FS_EP_MAXandTUP_DCD_ENDPOINT_MAXinsidetusb_mcu.h)
But IMO it is better to do them as a separate MRs (jira for the first one I have already created).
|
@roma-jam thanks for another valuable suggestions. PTAL at the changes so I can squash the commits. |
There was a problem hiding this comment.
@peter-marcisovsky Well done!
Please, squash the history.
Also, please update the PR description, according to the last changes (not macros but runtime EP verification an so on).
Otherwise, LGTM!
- Check if endpoint is avaliable while opening it
- Release endponint once is closed
peter-marcisovsky
force-pushed
the
feature/check_max_endpoint_count
branch
from
9dadcc0 to
1b81220
Compare
| TU_ASSERT(epnum < ep_count); | ||
| #if TU_CHECK_MCU(OPT_MCU_ESP32S2, OPT_MCU_ESP32S3, OPT_MCU_ESP32P4) | ||
| if (!dcd_edpt_available(rhport, dir)) { | ||
| TU_LOG(1, "No endpoints available (ep_max=%d) \r\n", dwc_ep_config[rhport].ep_max_count); |
There was a problem hiding this comment.
Would have been good to make this an error, spent some time to figure out I ran out of endpoints as this 'silently' passes... Before there was an assert.
There was a problem hiding this comment.
Hi @bollenn,
Sorry for the inconveniences.
Actually, the whole idea of that MR was exactly as you have said: to display the message when there are no more EPs available.
Before, there was the assert, but the comparison itself was between wrong values, so the assert never worked, even in case when there no more vacant EPs available.
May I ask, why is was "silent" in your case? TU_LOG was configured to 0?
There was a problem hiding this comment.
Can't remember, LOG level to low or return value check missing and this log just passing by in between a lot of other logs...
Change description
closes espressif/esp-idf#13005