Hello,I have created the draft merge request https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/49 so we can move the review process there.
Best regards, Michal On 23. 05. 24 7:17, Christian MAUDERER wrote:
Hello Michal, On 2024-05-22 19:49, Michal Lenc wrote:Hello, thank you for the feedback.Do you plan to add the subsystem to the RTEMS core or do you want to integrate it as an add-on library? In the first case, your code most likely gets more attention if you integrate it into RTEMS and create a (draft) merge request.The plan is to add it to the RTEMS core. Source code to cpukit/dev/can (with CTU CAN FD driver in ctucanfd subdirectory) and headers to cpukit/include/dev/can. So from this point of view, draft merge request is possible. I will take a look into it in few days.Regarding headers and their organization, we have done some code refactor for more sensible and comprehensible usage. I have described it in detail at web manual: https://otrees.pages.fel.cvut.cz/rtems/rtems-canfd/doc/can/can-html/can.html. Applications have to include can.h - this defines CAN frame structure, ioctls and other defines for API (some of these are included through other headers for better code structure, but the application just has to include can.h). Controller drivers include can-devcommon.h. These are mostly structures moved from can.h that are not use for API but only for the controller (can_chip structure, functions to filter frames to FIFO etc.). BSP that registers CAN bus into /dev namespace includes can-bus.h (+ controller specific header) which provides function can_bus_register().Great. Thank you.The plan is to install all of these headers even if they are not primarily intended to be used from an application. But we think it may be useful to provide the interface to create and register user-specific driver even from the application if someone has special needs surpassing the CAN stack capabilities.OK. Best regards ChristianBest regards, Michal On 21. 05. 24 9:09, Christian MAUDERER wrote:Hello Michal, On 2024-05-18 14:09, Michal Lenc wrote:Hello,Code review without patches or a review system is always a bit more effort because there is nothing to add comments directly. It seems that I can't register on the gitlab instance that you provided. So let's try it here.I already have an account on RTEMS gitlab so I can make my RTEMS fork and put the code there for next part of the review.Do you plan to add the subsystem to the RTEMS core or do you want to integrate it as an add-on library? In the first case, your code most likely gets more attention if you integrate it into RTEMS and create a (draft) merge request.Test or demo apps. So most likely not relevant for a review:Yes, those will not be going to mainline. I will write special testsuite application to test basic interface, similarly to SPI testsuite, for this.*Style*: I would suggest to group defines a bit more. You already used prefixes like RTEMS_CAN_QUEUE_* which is great. You can improve that a bit more if you use Doxygen "@name" and "@{" ... "@}". For an example take a look atAdded.*Question*: Why do you prefix some defines with RTEMS (like RTEMS_CAN_CHIP_MODE) and others don't have that prefix (like CAN_CTRLMODE_LOOPBACK)? The same is true for some other defines in other files. I won't mention it every time.We have tried to make a difference between generic CAN defines like modes, flags or errors and strictly RTEMS specific stuff like ioctl calls, queue directions and so on. I am not entirely sure whether our approach is correct, but there might be some benefits from having CAN defines without prefix (some of these defines may be even compatible between different systems).Sounds sensible.*Style*: Sometimes you use Doxygen @brief. Sometimes \ref. I think it works, but it's a bit odd.That's caused by my inexperience with Doxygen, changed \ref to @ref.*Question*: You have ioctls like RTEMS_CAN_DISCARD_QUEUES. According to the description, that ioctl has a parameter. Why is it an _IO and not an _IOW? The same is true for some more of the _IO ioctlsFrom Linux kernel documentation https://docs.kernel.org/driver-api/ioctl.html#command-number-definitions _IOW/_IOR is used if pointer to data is passed to/from kernel. If command with no argument or just integer argument is used, then _IO should be used. I have not found such a description in RTEMS, but I suppose the rules are the same?Yes, the same rules apply. I missed that there is an exception for integer arguments. So that's my fault.*Typo*: The description of RTEMS_CAN_GET_BITTIMING has a "geets" in it's comment.Fixed.*Note*: struct can_chip_ops doesn't have a description.Good point, added. I have also changed check_bittiming to check_and_set_bittiming, it is more fitting.*Detail*: can_bus_register(...) and can_bitrate2bittiming are missing a description. If they are a public interface, it would be good if they would have one.Added.*Detail*: Description of the can_frame_header. You have field descriptions like "This member holds the CAN ID value". My first thought was that it is some kind of address. But with taking a look at the code, it seems that it is a bit mask that is combined out of CAN_ERR_ID_* defines. If a field is expected to contain a certain group of defines: Can you add a note regarding that?Added. It is a CAN identifier in most cases, but the field is also used to store CAN_ERR_ID_* in case this is an error frame. Since CAN ID is only 29 bits, the difference between valid and error frame is determined by 31st bit of CAN ID (define CAN_ERR_ID_TAG). The complete description of CAN errors for the stack is here: https://otrees.pages.fel.cvut.cz/rtems/rtems-canfd/doc/can/can-html/can.html#error-reportingGreat. The doxygen description is a lot better now.*Style*: You have a group of defines like CAN_ERR_*_DATA_BYTE. On the first glance, I thought it would be the same group as CAN_ERR_ID_*. I think I would have used CAN_ERR_DATA_BYTE_* instead. Of course that's a style question and there are always good arguments for any style. Again: A doxygen group using @{ and @} might achieve the same.Yes, CAN_ERR_DATA_BYTE_* seems better, changed to it. Also I have added the groups.*Typo*: You have a CAN_ERR_PROT_LOC_DARA_BYTE instead of ..._DATA_BYTE.Fixed.*Question*: There are a lot of defines in can-frame.h like CAN_ERR_PROT_LOC_DATA or CAN_ERR_TRX_UNSPEC. Is it clear for someone more used to CAN, how to use these? Or would a description like "Possible values for the Byte xyz in a can message" help?Yes, these defines are mostly standard in CAN stacks (SocketCAN, NuttX etc.)OK. Then they don't need a detailed description.*Detail*: Again: I'm not happy with the descriptions of the fields of the structure. A field "flags" that is described as "This member holds CAN flags" isn't really helpful. Which values can I assign to that field? Is it a bit mask? Is it a field defined according to some standard? In that case even a "Holds standard CAN flags" would be useful because then I know that I just have to take a look at any CAN documentation.I detailed the description and added the reference to CAN frame flags.Great. Again: A lot better.*Note*: I don't like global defines like MAX_MSGOBJS without a prefix. That's polluting the name space. Is there a reason that it doesn't have the CAN or RTEMS_CAN prefix like all other defines?This is even an unused relict from previous design. Removed.Similar: There are defines like "BIT". Is there a reason for using such a generic name? If it (for example) helps porting existing drivers from another stack, that's great. Otherwise, I don't like these names.Some more in this file are: len2dlc, GENMASK, FIELD_PREP, FIELD_GETYes, these defines like BIT, GENMASK etc are there to match Linux kernel because of generated headers for CTU CAN FD controller (and will match other controllers in Linux kernel as well). Some of them are even used in NuttX https://github.com/apache/nuttx/blob/master/include/nuttx/bits.h.Perhaps the most ideal solution would be to move this to some generic include header as bits.h or bitfield.h to have those defined in RTEMS in general and not only for CAN stack.These are always a bit tricky. Moving them in a separate header sounds good. Whether that one is still a can/bits.h or a general RTEMS bits.h or something else most likely needs a bit of discussion.At the moment BIT is defined 7 times in RTEMS or libbsd. GENMASK is defined once in the "linux/bitops.h" in libbsd.len2dlc was renamed.*Note*: The doxygen documentation of struct canqueue_slot_t didn't work as expected: You used for example @next to describe the "next" field. That clearly didn't work: https://otrees.pages.fel.cvut.cz/rtems/rtems-canfd/doc/doxygen/html/structcanque__slot__t.htmlShould be fixed now.*Question*: You use the Atomic_Uint from rtems/score/atomic.h for the slot_flags. For new code, I would suggest using the atomic_uint from stdatomic.h instead (C11). You have included that file already, so it shouldn't be a problem.Changed.*Question*: Why is string.h included in that header? I don't see any str*, mem* or stp* functions used.Removed.*Detail*: can_bittiming_const has a name with a fixed 32 char length. In ctucanfd.c you initialize that with a constant string. Is there some reason to have a fixed length string in RAM instead of using a pointer to a constant string that can have an arbitrary length and can be (for example) in the Flash?Changed to const char*.Maybe I have a basic problem here: Which headers are (more or less) public ones (used to write drivers and applications) and which ones are internal only? Or in other words: Which headers will be installed?The general idea now is the user writing an application and developer writing a controller should only include can.h header and controller specific header (ctucanfd.h, virtual.h). The header dependencies are written in such way that all needed is included through can.h (probably best seen from doxygen dependencies https://otrees.pages.fel.cvut.cz/rtems/rtems-canfd/doc/doxygen/html/can_8h.html).There are not many functions programmer will use when writing driver, just the ones described here https://otrees.pages.fel.cvut.cz/rtems/rtems-canfd/doc/can/can-html/can.html#driver-interface. Some other functions are used from can-bus.c with ioctl calls (queue creation, discard, flush) and for read/write/open/close operations. Functions in can-queue.h are mostly called just from can-queue.c, but there is a possibility one might want to call them from the driver if he needs something really specific. In other words, there are not currently use publicly, but it is possible to use them if you need something special. It is a design philosophy taken from LinCAN, so I suppose Pavel Píša will have more to say about this.I will go through the description and comments and try to make it more comprehensible.Great. Thank you. Best regards ChristianThank you, Michal Lenc On 14. 05. 24 10:10, Christian MAUDERER wrote:On 2024-05-13 17:40, Christian MAUDERER wrote:Hello Pavel and Michal, sorry for the late reply. I was on vacation last week. On 2024-05-06 11:27, Pavel Pisa wrote:Dear Christian, On Tuesday 30 of April 2024 08:40:43 Christian MAUDERER wrote:For others, code under review hosted in CTU university GitLab server https://gitlab.fel.cvut.cz/otrees/rtems/rtems-canfd Documentationhttps://otrees.pages.fel.cvut.cz/rtems/rtems-canfd/doc/can/can-html/can.html https://otrees.pages.fel.cvut.cz/rtems/rtems-canfd/doc/doxygen/html/index.htmlMain developer behind extension to CAN FD and switch to RTEMS is Michal Lenc.The intention is to (hopefully) reach state when it meets criteriato mainlining int RTEMS CPU kit under cpukit/dev/can...I agree, that it is compromise. But adding yet another file descriptor like multiplexor for queues to each file descriptor seems to me as too much complexity. But it can be added. even later as IOCTL to removeindividual queues based on CAN ID matches or queues IDs if create is modified to return internal queue IDs...I somehow missed that you can open the device multiple times and get independent queues. With that, it's completely OK and should be flexibleenough for most applications.It's great that you already have put some thought into how it could beextended later if some application needs more flexibility....Did you check withsome other hardware controller, whether the whole structures / defines / flags close to the hardware do work well for other controllers too?The code/concept is based on my previous LinCAN and OrtCAN work https://ortcan.sourceforge.net/lincan/...I didn't want to doubt your competence. Like I said it's some trap that I have fallen into often enough myself (like when guiding Prashanths GSoC project). But it's clear that you have put a lot of thought into that. So I would expect that there shouldn't be much trouble with most controllers. Maybe except for the ones where a semiconductor vendorthought it would be a good idea to create a completely different concept. But these are always difficult.I agree with discussion and searching for hard arguments. The solution is compromise and in general CAN bus concept is optimized for direct replacement of wires in car going between distinc units and its use as general communication solution has some difficulties and requires some compromises. For small devices with predefined purpose and Autosar, it is ideal to allocate for each CAN ID (wire signal) to be sent one communication object on the controller. Same for each received signal value or their set in the single frame. The most controllers are equipped by filters and mechanism to do so including selection of the Tx message object for physical bus-link arbitration according to the priority. Then sending side updates signal value in corresponding Tx object and receiving side sees most actual one usually on the best effort basis, older unread frames are overwritten by updated value. But even in simple ECU, there are obstacles to use this principle in all kind of the communication. CAN bus is used for firmware updates and general configuration. In this case, the reliable delivery of all messages with given CAN ID is required because whole sequence has to be received and processed and the state evolution is associated to the sequence. If a single message is lost, then all data are unusable. Because sequence requires exact ordering it is typical that only single Tx object is used. On Rx side there can be problem to capture all frames without overwrite by single Rx object so some controllers ad FIFO which can be attached to each object or some mechanism how to allocate more Rx objects and pass them to the user in FIFO order. That works for small ECUs with single purpose firmware. But on general purpose operating system which should allow even complete monitoring of the CAN bus, allows dynamically started applications and even whole virtual CAN/CANopen nodes, allocation the controller Tx/Rx message objects for each specific purpose is impossible. That is why all generic CAN subsystems which I know (CAN4Linux, LinCAN, SockteCAN, NuttX char device CAN, windows Peak's drivers etc.) define API based on opening driver and presenting received messages in FIFO order to application (with options for software filtering but usually not propagated to controller, HW - LinCAN has some option to union user FIFOs to mask and ID propagated to HW, but you usually end with fully end with need to receve all anyway and it has not been used at the end). The Tx FIFO order is required for messages with same ID or even sometimes between same stream of mesages even wit altering ID for correct realization of some higher level protocols. The result is that even on hardware equipped with multiple Tx objects but without special Tx FIFO order preserving cyclic queue only single Tx object is used to realize transmission of all messages, for example SocketCAN on XCAN controller. So only part of the CAN bus media badwidth can be utilized by single node. May it be, it is sometimes a luck, because CAN IDs are not correctly allocated accordingto priority even on cars critical subsystems. On the Rx side originalbuffers approach is hard to use in order preserving FIFO concept, but the most of today controllers add some option to keep order and leave processing and distribution on software side. See evolution from CCAN to DCAN to overcome that problem. We have even made LinCAN for CCAN many many years ago which somehow kept required properties but it was headache. So back to generic OS can interfaces, all I know are FIFO(s) based. Most of them keep strict FIFO order on Tx side which results in HoL (head-of-the-line) blocking and priority inversion on bus loaded by middle priority from other node.That is why SocketCAN adds alloc_candev_mqs (multiple-queues) alternativefor drivershttps://elixir.bootlin.com/linux/latest/source/drivers/net/can/dev/dev.c#L249but as I know, no mainline kernel driver is using that. We have done some work to research and even a little extend Linux networking QoS subsystem to solve buffer bloat by old messages for traffic requiring best effort (most up to date data for control) for given IDs and to limit badwidth of others or virtual guests connected through QEMU to physical bus etc. may years ago at time when multi-queue has not been available on Linux side. I have long time plan to extend CTU CAN FD mainline Linux driver for this support and probably to be the first example how to overcome HoL/priority inversion in Linux CAN subsystem. It has been planned in original LinCAN before SoketCAN and it is now implemented in proposed RTEMS CAN/FD framework where application can setup multiple queues even for single open instance with different Tx priority class and when used and mapped correctly to CAN IDs, it can prevent priority inversion. It is not generic, because it is quite expensive for deeper FIFOs and even mutual order of Tx messages has to be preserved for many protocols as discussedearlier. CTU CAN FD IP core interface to software has been architectedby me to allow maximal utilization of the Tx buffers and their reallocation when needed for higher priority message. Wait for DTP processing and publication of our international CAN Conference 2024 article or come and meet next week in Baden-Baden https://www.can-cia.org/icc/ There are two branches of the thought from this point 1) how it maps to other controllers For these equipped by single Tx object only (i.e. SJA1000), it maps well because attempt to repeat Tx and arbitration can be disabled when higher priority queue becomes ready and our CAN infrastructure allows to push back lower priority message and schedule higher one to be sent. For more complex one, if they do not allow to control Tx objectsorder then only single Tx object can be used. Bad, link underutilization,but it is what is standard in SocketCAN and other CAN solutions for general purpose operating systems today. All controllers which I know allows to stop Tx attempt repeat and I hope to seen at all option t check if the latest attempt has been successful or not. So newt RTEMS CAN can use them same as on SJA1000. On Rx side, most have FIFO preserving option to use multiple buffers. Sometimes partially broken, burdened by erratas etc. (like iMX RT where we overcome these problems in NuttX drivers). When number of Tx priority classes is limited (for proposed system by default 3 but compile time configurable) then we can allocate one Tx buffer for each class, easy and preserves HoL priority inversion even on simple controllers. If there is option to order Tx according to the buffer index in the controller, then there is option for a little more performant solution when multiple Tx buffers are allocated for each class and they are sequentially filled till highest allocated buffer index is filled. Then there is some gap till all these buffers in given priority are sent because cyclic filling of the minimal index would result in reordering with possible break of some protocol requirements. Some controllers allows to attach DMA realized FIFOs to more Tx objects, in such case it would map to proposed design well too. Some newer controllers adds local priority bits above CAN ID ones (i.e. new NXP FlexCAN). This could allow cyclic use of some Tx objects/buffers similar to CTU CAN FD. There will be problems because multiple Tx buffers priorities are not reachable by single atomic operation like in CTU CAN FD case. But I have some idea how to implement sequential updates to ensure order in the class. There would be problem, that most controllers do not allow to update this information on the objects participating actively in arbitration. So it would lead to much more acrobation between eggs and some gap time, where none message is offered in the link arbitration even that there are pending user requests will be inevitable in some scenarios after some number of messages sent. That cannot be on the bus side worse that considering fixed order according to index. May be, it can be found that overhead does not worth that. But we preserve API in variants in all cases... 2) use of the CAN bus in applications requiring maximal bus transparency with minimal latency and SW load. This is totally opposite of the general CAN bus subsystem for general purpose RTOS. The API in this case should allocated Tx and Rx controller objects for the individual purposes/CAN IDs. Rx side SW processing can be considered as alternative and proposed framework allows to setup queues, but it has overhead and underextreme load it can lost some messages if HW is not performant enough.On Tx side it is even more problematic.But if this type of use of RTEMS for example for Autosar or Simulinkgenerated code is considered then it is possible to extend actual proposed API by IOCTLs which allows to reserve some controller objects for specific purposes and allows to access them directlyfor minimal overhead and use under direct application control or attachseparated controller side "canque_ends_dev_t" to such objects and propagate them to some clients to standard CAN read and write API. So I think that the proposed framework provides what is expected bu most of general purpose CAN/CAN FD framework users, tries to perpare a little even for come of CAN XL, solves problems which may be practically unsolved by all other generic approaches still. And we have some clue how to extend support for most/all other controllers and even some open doors to offer even ECU style API for applications which benefit from direct controller buffers use/allocation which is possible on controllers with abundant number of buffers (not case of SJA1000 and very limited on CTU CAN FD - max 8 can be configured to silicon under actual registers map). I understand that the text is long but you have asked for it in the fact and I provide complete thought dump to analyes it.Thanks for the (very) detailed explanation. My intention was to express that I'm completely OK with only one driver because you clearly have thought about other hardware too. Your explanation just makes it even more clear how much thought you put into it ;)I would be happy if you and or others find time to look into actual code implementation to identify what could be issue for mainlining as soon as possible because after May 24 changes do not propagate into Michal Lenc's thesis text which can be alternative and more in depth documentation and analysis than what fits into official RTEMS one. The full document has already 47 pages and 34 of the actual text without content and appendices. Document includes benchmarks under RTEMS load by HTTP traffic, priority inversion prevention confirmation by measurements with performance data etc. It will be published on CTU in May or June https://dspace.cvut.cz/ and links will be added to https://canbus.pages.fel.cvut.cz/ same as for much shorter iCC article and presentation.Code review without patches or a review system is always a bit more effort because there is nothing to add comments directly. It seems that I can't register on the gitlab instance that you provided. So let's try it here.I'll mainly take a look at the headers because they define the interface. That's the most important part if you ask me. Bugs in the code should be fixable later.I'll try to categorize my comments a bit. If it has a *Style* or *Typo* in front of it, you can just ignore it. It's not really important. It's just something that I noted while reading through the code. *Question* or *Note* are more important.And please note: You know CAN a lot better than me. So quite possible that I don't see a lot of stuff and that I might have some odd odd questions.### First the ones that I plan to more or less ignore so that I can concentrate on the important parts:Test or demo apps. So most likely not relevant for a review: ./rtems_can_test/can_test.h ./rtems_can_test/app_def.h ./rtems_can_test/system.h ./rtems_can_test/can_register.h ./zynq_template/app_def.h ./zynq_template/zynq_reg.h ./zynq_template/system.h Seems to be left over from some tests: ./lib/libbar/bar.hDriver specific files. I think these are not that high priority either:./lib/candrv/ctucanfd/ctucanfd_txb.h ./lib/candrv/ctucanfd/ctucanfd_kframe.h ./lib/candrv/ctucanfd/ctucanfd_kregs.h ./lib/candrv/dev/can/ctucanfd.h ### Now the more important ones: The interfaces #### ./lib/candrv/dev/can/can.h*Style*: I would suggest to group defines a bit more. You already used prefixes like RTEMS_CAN_QUEUE_* which is great. You can improve that a bit more if you use Doxygen "@name" and "@{" ... "@}". For an example take a look athttps://gitlab.rtems.org/rtems/rtos/rtems/-/blob/main/cpukit/include/dev/i2c/i2c.h?ref_type=heads#L80Which leads to a group in the doxygen output:https://docs.rtems.org/doxygen/branches/master/cpukit_2include_2dev_2i2c_2i2c_8h.htmlThe same is true for some other defines in other files. I won't mention it every time.*Question*: Why do you prefix some defines with RTEMS (like RTEMS_CAN_CHIP_MODE) and others don't have that prefix (like CAN_CTRLMODE_LOOPBACK)? The same is true for some other defines in other files. I won't mention it every time.*Style*: Sometimes you use Doxygen @brief. Sometimes \ref. I think it works, but it's a bit odd.*Question*: You have ioctls like RTEMS_CAN_DISCARD_QUEUES. According to the description, that ioctl has a parameter. Why is it an _IO and not an _IOW? The same is true for some more of the _IO ioctls.*Typo*: The description of RTEMS_CAN_GET_BITTIMING has a "geets" in it's comment.*Note*: struct can_chip_ops doesn't have a description. I'm not entirely sure what every member should do. For example: check_bittiming: From the name I would expect that it only checks a bit timing. From the ctucanfd.c it also sets a bit timing. I think it would be good if you would add short descriptions here like "Check and set a bittiming. Returns 0 on success or negated errno on error."*Detail*: can_bus_register(...) and can_bitrate2bittiming are missing a description. If they are a public interface, it would be good if they would have one.#### ./lib/candrv/dev/can/can-frame.h*Detail*: Description of the can_frame_header. You have field descriptions like "This member holds the CAN ID value". My first thought was that it is some kind of address. But with taking a look at the code, it seems that it is a bit mask that is combined out of CAN_ERR_ID_* defines. If a field is expected to contain a certain group of defines: Can you add a note regarding that?*Style*: You have a group of defines like CAN_ERR_*_DATA_BYTE. On the first glance, I thought it would be the same group as CAN_ERR_ID_*. I think I would have used CAN_ERR_DATA_BYTE_* instead. Of course that's a style question and there are always good arguments for any style. Again: A doxygen group using @{ and @} might achieve the same.*Typo*: You have a CAN_ERR_PROT_LOC_DARA_BYTE instead of ..._DATA_BYTE.*Question*: There are a lot of defines in can-frame.h like CAN_ERR_PROT_LOC_DATA or CAN_ERR_TRX_UNSPEC. Is it clear for someone more used to CAN, how to use these? Or would a description like "Possible values for the Byte xyz in a can message" help?#### ./lib/candrv/dev/can/can-filter.h*Detail*: Again: I'm not happy with the descriptions of the fields of the structure. A field "flags" that is described as "This member holds CAN flags" isn't really helpful. Which values can I assign to that field? Is it a bit mask? Is it a field defined according to some standard? In that case even a "Holds standard CAN flags" would be useful because then I know that I just have to take a look at any CAN documentation.#### ./lib/candrv/dev/can/can-devcommon.h OK. #### ./lib/candrv/dev/can/can-helpers.h*Note*: I don't like global defines like MAX_MSGOBJS without a prefix. That's polluting the name space. Is there a reason that it doesn't have the CAN or RTEMS_CAN prefix like all other defines?Similar: There are defines like "BIT". Is there a reason for using such a generic name? If it (for example) helps porting existing drivers from another stack, that's great. Otherwise, I don't like these names.Some more in this file are: len2dlc, GENMASK, FIELD_PREP, FIELD_GETNow I'm running out of time. I'll try to take a look at the following files later or tomorrow:Like promised: #### ./lib/candrv/dev/can/can-queue.h*Note*: The doxygen documentation of struct canqueue_slot_t didn't work as expected: You used for example @next to describe the "next" field. That clearly didn't work: https://otrees.pages.fel.cvut.cz/rtems/rtems-canfd/doc/doxygen/html/structcanque__slot__t.html*Question*: You use the Atomic_Uint from rtems/score/atomic.h for the slot_flags. For new code, I would suggest using the atomic_uint from stdatomic.h instead (C11). You have included that file already, so it shouldn't be a problem.*Question*: Why is string.h included in that header? I don't see any str*, mem* or stp* functions used.*Question*: Some of the functions have a bit of a short description. For example the canqueue_filter_match: The brief description basically just tells me exactly what the name of the function already told me. But how and in what situation would I use that function? How do these filters work? I can't tell that easily from the description or from the implementation of the function. Is it even thought as an interface that a user (in this case: someone writing a driver or an application) has to understand?Maybe I have a basic problem here: Which headers are (more or less) public ones (used to write drivers and applications) and which ones are internal only? Or in other words: Which headers will be installed?For this file, I strongly suspect that it is not user-facing. You have a lot of undocumented functions in it (like canqueue_next_inedge or canque_for_each_inedge).The files that are really relevant for review at the current point are mainly the ones that a user (again: someone writing a driver or an application) can see.If it is a public facing header: Did I miss some general description how a filter works and how a user should use it? It's quite possible that I missed that. I'm still only scratching the surface of your work at the moment.#### ./lib/candrv/dev/can/can-stats.h Looks OK. #### ./lib/candrv/dev/can/can-virtual.h OK. #### ./lib/candrv/dev/can/can-bittiming.h*Detail*: can_bittiming_const has a name with a fixed 32 char length. In ctucanfd.c you initialize that with a constant string. Is there some reason to have a fixed length string in RAM instead of using a pointer to a constant string that can have an arbitrary length and can be (for example) in the Flash?Best regards ChristianBest regards ChristianBest wishes, Pavel -- Pavel Pisa phone: +420 603531357 e-mail: p...@cmp.felk.cvut.cz �� Department of Control Engineering FEE CVUT Karlovo namesti 13, 121 35, Prague 2 university: http://control.fel.cvut.cz/ personal: http://cmp.felk.cvut.cz/~pisa company: https://pikron.com/ PiKRON s.r.o. Kankovskeho 1235, 182 00 Praha 8, Czech Republic projects: https://www.openhub.net/accounts/ppisa social: https://social.kernel.org/ppisa CAN related:http://canbus.pages.fel.cvut.cz/ RISC-V education: https://comparch.edu.cvut.cz/ Open Technologies Research Education and Exchange Services https://gitlab.fel.cvut.cz/otrees/org/-/wikis/home
_______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
