Adaptations for Renesas CCRX toolchain and Rx72N controller performed#859
Conversation
|
Thank you very much for your effort to port the ccrx toolchain. I am not familiar with renesas toolchain, does it has trial with limited functionality ( I don't need all the features), only enough so that I could test this out hand-on before merging and possibly help with tweaking the implementation. PS: this PR come at pretty good time, my rx65n cloud kit board just come, it will come handy, I still need to get the board ported those. But it is probably similar to existing rx72n. |
|
Yes there is an evaluation version of the toolchain available. The trial period is the 60 days from the day the first build is executed after installing an evaluation edition. After the trial period exceeded the linkage is limit to 128 KB or smaller. Here is the link to toolchain webside : CC-RX The USB HW implementation of the Rx65N is equal to the one found on the Rx72N. BTW: I also have an evaluation board with the Rx65N, the Envision Kit RPBRX65N. |
|
thanks, that is great to hear, most example is probably less than 128 KB limit. I will give it a try, yeah, the envision seems to be a good kit, though I think it doesn't route the USB to connector and requires additional add-on board from what I could understand. Update: hmm, I couldn't find the toolchain for Linux host, do you know if they only have ccrx on windows only ? |
|
The Envision Kit HW only offers the host controller functionality (on a USB-A connector). On my Rx72N Envision Kit (which is equal as the Rx65N Envision Kit around the USB HW) I had to modify my board slightly, so it could be used as a function controller (remove the ISL6186USB Port Power Supply Controller and add a resistor). Unfortunately there is no CCRX toolchain version for Linux (I use a Windows VM on my Linux host). Only a Linux version of their e2studio IDE (this is an eclipse based IDE) is available, of course the GCC toolchain could be used with. |
Yeah, that is why I went to the cloud kit although I have no need for ESP32 co processor.
That is a pain, I will try to run it on vm as well, could take a bit for set up since my VM is not well installed for developing. Will get back to this asap. |
|
still on my radar, just too busy with paid work. Please be patient. Meanwhile can you merge from master to resovle the conflict |
hathach
left a comment
There was a problem hiding this comment.
Thank you very much for your effort putting into this PR. This is lots of work and testing, there is also fixes to existing rx driver as well. However, the weak function walk-around is not good enough and is a total deal breaker. It is totally not your fault but rather mine for choosing the weak function for user convenience and assume that most toolchain would support it.
I think we either have to come up with another walkaround for weak function or have to merge other changes first. And then think of way to address weak function as separate/follow up PR.
PS: I have a question, if instead of having weak as optional, how about we force all project compile with ccrx to define all the function. e.g even if it is not used/implement, application still have to declare the function. Then they are all "strong" and normal function, and that will work as others without having to have an walkround. It is like an strict requirement for ccrx, but it is not too difficult to do. Does this walkaround work in your opinion ?
| uint8_t bDescriptorType ; ///< Descriptor Type, must be Class-Specific | ||
| uint8_t bDescriptorSubType ; ///< Descriptor SubType one of above CDC_FUCN_DESC_ | ||
|
|
||
| TU_BIT_FIELD_ORDER_BEGIN |
There was a problem hiding this comment.
the bit field order is default to right, maybe we don't really need to define this. If you still want to define this, I think it is best to just put only one at the beginning of the struct declare, and one at the end like struct begin above.
There was a problem hiding this comment.
Putting the bit field ordering at the beginning and the end of a structure declaration is fine for me.
BTW: I'm not sure if the bit field order is really important on every bit field. I did not haven't a deeper look on it, if it is a local used bit field. I guess it is only important, if the bit field is transferred over USB or if it is mapped over a RAM/HW region and not if it is used as a local status variable.
There was a problem hiding this comment.
This only need to apply to those in usb specs, local vaiable should have no problem. However, the default right is the same as the default of gcc. So I think we are good to start with.
| //--------------------------------------------------------------------+ | ||
|
|
||
| /// Header Functional Descriptor (Communication Interface) | ||
| TU_PACK_STRUCT_BEGIN |
There was a problem hiding this comment.
There is a series of packded struct, I think we should simply this by putting only 1 pack begin and 1 pack end for the all struct define. Just put an comment to tell we need to remind us is good enough.
There was a problem hiding this comment.
Yes, that is possible. The only exception, which comes to my mind at the moment, is that it could be a negative speed impact if accessing packed structures. Maybe we should check if the packed attribute is really required on all structs here ?
There was a problem hiding this comment.
we only group these packed structure that are consecutive to each other in tusb_types.h i.e we only group them together instead of declaring for each struct. So it is literally the same.
| #define ADD_WEAK_FUNC_TUD_CDC_RX_CB 0 | ||
| #define ADD_WEAK_FUNC_TUD_CDC_RX_WANTED_CB 0 | ||
| #define ADD_WEAK_FUNC_TUD_CDC_TX_COMPLETE_CB 0 | ||
| #define ADD_WEAK_FUNC_TUD_CDC_LINE_STATE_CB 0 | ||
| #define ADD_WEAK_FUNC_TUD_CDC_LINE_CODING_CB 0 | ||
| #define ADD_WEAK_FUNC_TUD_CDC_SEND_BREAK_CB 0 | ||
|
|
||
| // "Weak function" emulation defined in usbd_pvt.h | ||
| #define ADD_WEAK_FUNC_USBD_APP_DRIVER_GET_CB 0 | ||
|
|
||
| // "Weak function" emulation defined in usbd.h | ||
| #define ADD_WEAK_FUNC_TUD_DESCRIPTOR_BOS_CB 0 | ||
| #define ADD_WEAK_FUNC_TUD_DESCRIPTOR_DEVICE_QUALIFIER_CB 0 | ||
| #define ADD_WEAK_FUNC_TUD_MOUNT_CB 0 | ||
| #define ADD_WEAK_FUNC_TUD_UMOUNT_CB 0 | ||
| #define ADD_WEAK_FUNC_TUD_SUSPEND_CB 0 | ||
| #define ADD_WEAK_FUNC_TUD_RESUME_CB 0 | ||
| #define ADD_WEAK_FUNC_TUD_VENDOR_CONTROL_XFER_CB 0 | ||
|
|
||
| // "Weak function" emulation defined in dcd.h | ||
| #define ADD_WEAK_FUNC_DCD_EDPT0_STATUS_COMPLETE 0 | ||
| #define ADD_WEAK_FUNC_DCD_EDPT_CLOSE 0 | ||
| #define ADD_WEAK_FUNC_DCD_EDPT_XFER_FIFO 0 |
There was a problem hiding this comment.
This is troublesome, I don't think we could replicate this for all the stack, nor I could keep it consistent after this PR si merge. And explicitly requires user to manually enable each callback is too much. This is deal breaker,
There was a problem hiding this comment.
Ok, I understand that. The advantage of this would be, that the compiler does not generate code if the callback is defined as NULL.
There was a problem hiding this comment.
It is not convenience to force user to manually enable this one by one. Also the modification is too much to support and maintain. There are dozens more used by other class driver and host stack in the future. For ccrx, without weak support, I guess this is good compromise.
|
Of course, the easiest way would be to leave the weak macro for the CCRX port undefined. It would the be required by the user to implement all the functions which have the weak attribute in the GCC port. But I can not estimate does this have any impact on the execution speed (means calling empty callback function all the time) ? If I see a negative impact on the project I'm using tinyUSB, then I might make a local change to overcome the problem for the time being. |
It should be that much, if that becomes an issue, you can actually can add |
|
If the compiler does not support a weak feature, why not call the default inline callback function specified by the compiler argument? I tried that the following experiment. #include <stdio.h>
static inline void default_cb(void){}
int main(void) {
if (hello) {
printf("ok\n");
hello();
}
return 0;
}I compiled the above file which name is hello.c with following command. If an application no need some call back function, a macro should be defined as default inline callback function specified by the compiler argument in the build script (Makefile) . And all class drivers must have a default callback defined. |
|
inline function may not work with if (weak_func) weak_func()Since it does not emit symbol. Also the static inline require function must be included by the class driver, and which means all the weak function must be implemented in header file of For slow execution, ccrx probably smart enough with -Ofast to know that the function is empty and just omit that, not entirely sure though. |
|
I understood @hathach's opinion, and I agree with to force the strong symbol on CCRX. If application developers are concerned about calling empty functions on CCRX, they can achieve the trick I suggested by defining a static inline function in the application's |
|
@hathach Some note about the missing weak attribute on CCRX. I tried to cleanup the code and removing my solution of the weak functionality. While doing so I found the following code situation: Lines 998 to 1008 in 1b84739 If I implement an empty BTW: I have not checked yet if there are other similar situations in the code. |
|
just return NULL |
|
@hathach Removed the missing weak attribute work around and merged again from current master |
|
|
||
| static inline osal_queue_t osal_queue_create(osal_queue_def_t* qdef) | ||
| { | ||
| #if defined(__Tx36V5_Maincard__) |
There was a problem hiding this comment.
please remove all the platform dependent __Tx36V5_Maincard__
There was a problem hiding this comment.
Switching from CFG_TUSB_OS=OPT_OS_FREERTOS to CFG_TUSB_OS=OPT_OS_CUSTOM to use my own RTOS API integration file to overcome this issue
| typedef StaticSemaphore_t osal_semaphore_def_t; | ||
| typedef SemaphoreHandle_t osal_semaphore_t; | ||
|
|
||
| #if (configSUPPORT_STATIC_ALLOCATION == 1) //FIXME Only static API supported |
There was a problem hiding this comment.
can it possible that you just enable configSUPPORT_STATIC_ALLOCATION, the dynamic API can still be used with this option turned on.
There was a problem hiding this comment.
Switching from CFG_TUSB_OS=OPT_OS_FREERTOS to CFG_TUSB_OS=OPT_OS_CUSTOM to use my own RTOS API integration file to overcome this issue
There was a problem hiding this comment.
FreeRTOS can have both dynamic and static co-exist, I am curious why don't you just enable the static allocation. Sure I could update the osal to support dynamic api from freeRTOS as well, but yeah, it should be on its own PR.
|
|
||
| static usbd_control_xfer_t _ctrl_xfer; | ||
|
|
||
| #if defined(TU_HAS_NO_ATTR_ALIGNED) |
There was a problem hiding this comment.
does rx mcu requires the usb buffer to be 4 byte aligned, if not this can be ignored
There was a problem hiding this comment.
As I understand it, there is no need for alignment if DMA is not used. And the current implementation does not use DMA.
If using DMA, the buffer must be 2 bytes aligned.
There was a problem hiding this comment.
RX MCU does not need an alignment on this. It was introduced, because there was an error in dcd_usba.c, which is corrected. On my test code the _usbd_ctrl_buf buffer started on an odd address and with the error in dcd_usba.c the transfer over an endpoint was faulty. So this fix is no longer required (as long as no DMA transfer is supported).
There was a problem hiding this comment.
It is surprised that ccrx has align pragma for function but not variable. Anyway there is several other places that may requires alignment especially endpoint buffer in each class driver. Therefore, maybe we revert this changes for now, and will revisit these later when doing DMA. SInce doing this alone is not enough anyway.
| const uint32_t addr = USB0.USBADDR.BIT.USBADDR; | ||
| if (!addr) return; | ||
| #if defined(__CCRX__) | ||
| tusb_control_request_t setup_packet; |
There was a problem hiding this comment.
oh, ccrx does not support this style of initialization ? Hmm, there is a lots more of this within codebase.
There was a problem hiding this comment.
This is the style of the C99 standard. CCRX (v3.03) supports the C99 standard by specifying -lang=c99 as a command line argument. Since TinyUSB implicitly requires c99, I think CCRX should also have this option.
There was a problem hiding this comment.
yeah c99 is really old now, it should be enabled to build with.
There was a problem hiding this comment.
Don't worry. I found the solution that also works with GCC, so the check which toolchain is used is no longer required. The problem with this struct definition and CCRX is, that one of the struct member is defined as an union which includes some bit fields (that could be an error of the CCRX toolchain ? ).
BTW: I already use C99 language option.
|
@Wini-Buh |
|
I want to help to speed up the merge of this PR (has been going for a while), so that #874 can be merged, and also help testing with the net lwip example. However, setting up the rn65n cloud kit (picked because it come with add-on board with usb connection) is a bit difficult to me since this is first time I worked with renesas mcu. Renesas driver is either too bare metal or too complicated with FIT module (I don't really like generated code sigh). Will give it more try, any advice is much needed :) |
|
@hathach Maybe I could support you a little bit. I don't also like the FIT modules (I had to much negative experience with it, like with other similar technique from other vendors). Is there a board support package available for your board ? It would at least help you to speed up the clock programming for the first time. BTW: Sorry for the time delay about this PR, but at the moment I'm very busy at work. |
|
@kkitayam bravos, that is a great progress 👍 👍 |
# Conflicts: # src/portable/renesas/usba/dcd_usba.c # src/tusb_option.h
|
The compilation error with GCC should be solved. I did not realize that the GCC throws an error on this. So there were only two solutions. Use a toolchain dependent compilation (like I did it now) or try to temporary disable the error while compiling (don't know how with GCC... only know it on CCRX). |
|
@Wini-Buhhow I am still catching up with this PR. Would you compile rxcc on windows with stock example, I tried to use make command, however, since rxcc use different compile and linker option from command line than rxgcc and I don't understand it enough to make change to makefile to get it compiled. I guess you are having an e2studio project file, It would be great if you could share the file so that I could compiler and test it out. |
|
@hathach I will try to build a "stock example" with the e2studio and give you the project file. But I need to do that at the weekend and I can't test the example because I do not own the board used for the RX examples. In the meantime I show you the arguments given to the compiler, assembler and linker so you could see the how the command line options look like. You should find the description of each argument in the manual of the toolchain. |
|
thanks, a project file to compile with stock example is very useful, if you could make one for cdc_msc (for testing) then the net_lwip_server as well so that I could help. The example for compile option txt is useful, I wish I know these well enough to put that into makefile to easily switch between rxgcc and rxcc. But I don't think I could pull it off, and could be error prone. It is better to address one thing at a time and stick with things that work first. |
|
@hathach Here are the two e2studio project files of the cdc_msc stock example. I setup it up for a RX65N Cloud-Kit, which unfortunately I didn't own. That means I could not test it. I could only compile it with the CCRX. After all I'm not sure if it is very useful for you, but please have look at it. |
|
@Wini-Buh late response, I tried to import your project files, though look like it expect some certains file structure, and also missing some of your local files to build. This is why I don't like from IDE, it takes lots time to configure and setting up for an port/board and may not portable to other users. Anyway, since I got the gccrx working well now, I will try to update PR to my best effort so that we could merge this soon. Since the pr has been pending for so long, and have some good addition. For rxcc I will reply on you to do the actual testing. |
…Wini-Buh-CCRX_Port
I notice from you have |
clean up packed/bit order begin end
Ok. Great to hear you got CCRX working well now. I mentioned that the project file alone maybe is not very useful for you.
That is because I used one of my projects based on the project I'm working on (and that is using tinyUSB). This project uses a lot of existing code (based on the SH2A controller from Renesas) and there the default bit order is left. That's the only reason. |
Actually not rxcc, the gccrx though, sorry for not being clear enough. That allows to make changes, mostly rename compiler attribute and other minor clean up with confidence. I have push update to your fork to update PRs. Please pull first if you want to do further work with it.
Ah, i see, that makes sense. I am just curious why would you need that. |
hathach
left a comment
There was a problem hiding this comment.
It has been pending for awhile, although I couldn't get tested with ccrx, the modification look good. It also contains bug fix when using the stack with big-endian mcu (which isn't tested yet). I think it is best to merge it for now, and resolve the net_lwip_webserver example with ccrx as follow up PR later on. That example is complicated, there may need some config tweak with lwip (much like tinyusb) to work with rxcc as intended as well.
I did a bit of rename and cleanup (please do pull if you plan to make more changes to the PR). There is only a few feedback as follows:
- revert change to wMaxPacketSize field until tested if you don't have time to do it for now.
- confirmation for removal of odd address check in fifo_write() of dcd
- confirmation for inverted logic of statement
while (USB0.D0FIFOSEL.BIT.CURPIPE != num)
@kkitayam if you have some time, would you mind help to confirm the changes, it is your great work, you understand this much better than me.
hathach
left a comment
There was a problem hiding this comment.
Thank you very much for your PR. Let merge this as it is now, since it is good enough. For more issue with ccrx and net driver, we could resolve it later on as folllow up PR when having more time to work on it.

As discussed in #785 this a draft PR for the port of tinyUSB to the CCRX toolchain and the RX72N device from Renesas.
It is still not complete and only the CDC device class is fully ported. But I guess it is good enough for a discussion.
The following points had to be overcome for porting it to the CCRX toolchain (and the Rx72N used in my project):
Because the FreeRTOS implementation only support the "...Static" API and I use the "non-Static" API and a slightly modified task switching mechanism, I had to add/change some code. Please ignore that.
BTW: I have setup a development environment to build a sample project (with the Rx72N) with following configurations:
With this environment, I can easily check if all configuration produce the same results