-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
problem with nxsig_timeout (sig_timedwait.c) #740
Comments
I have found a same problem which Sebastian pointed out in another processor. So, I have investigated the all callers of wd_start() in the latest NuttX kernel. Many of them have no need to fix because they are for the specific architecture 7 callers pass the function pointer with only one argument.
The only one exception is pthread_condtimedwait() which handles two arguments. To make it portable, va_arg() must be used in pthread_condtimedout(). I will issue the PR for them. |
The minimum essential changes for the portability are included in the PR, but My proposal is, to change the API to: And change the typedef of wdentry_t to: Instead of passing the variable arguments, pass the pointer to struct and its size How about the proposal? I think that it is difficult to change such API spec, but if acceptable, |
@YuuichiNakamura do you mean to make wd_start do memcpy with the given size to save the structure? |
Yes. |
the proposed api looks better than the current one to me. |
I prefer the existing interface. Not only is the proposed interface very ugly but does not seem to have any purpose other than added complexity. Let's not change this; we are better off without that change. |
That is odd since the AVR architecture has been supported by NuttX for over ten years and has been thoroughly excersizes with no known, oustanding problems. I believe the existing interface works fine with AVR. |
Please don't. I will close it without merging. It is inappropriate and undesirable. |
@patacongo I understood. It's ok for me if the PR #741 is accepted. |
@patacongo the current api has at least two issues, besides the one pointed out here.
|
i agree the proposed api is a bit ugly. but it's definitely better than the current api. |
What controls the "life" of structure pointed to by "arg". It cannot be in a stack and it can become stale if it is in allocated memory. In general, it cannot be a static variable either. If allocated, what frees the structure. Because of these complexities, I think this is a bad idea. Let's not do this. I vote -1. Why not just:
Since those are passed directly be value rather then through a container structure, the interface is simplified and there is no concern for management of the life of an allocated structure. |
This is not a problem, however, if you copy the data out of the structure and into the wdog_s structure immediately in the wd_start call as is done now. The original structure itself must be assumed to be volatile. |
The inefficiency in the proposal should be considered as well. You once told me that all calls to wd_start() but one provided only a single parameter. it sems wasteful to set up a structure containing that one parameter and passing it to to wd_start(). What is the size argument? There can be no more than CONFIG_WDOG_MAXPARMS arguments and each must be size wdparm_t. A size_t is used to hold the largest possible memory object (in bytes). A count of arguments makes more since and should be just an int. So If I wanted to pass one argument I would have to do:
arg would have to copied to a wdparm_t variable because sizeof(arg) may not be compatible with sizeof(wdparm_t) so you cannnot just pass a pointer to arg. Then wd_start() would have to divide size by sizeof(wdparm_t) to get the number of arguments? Versus my suggestion: ret = wd_start(&wdog, delay, entry, arg, 0); Since we are passing by value, not reference, arg will be automatically converted to type wdparm_t. Which looks less "ugly" to you. (We should not be using ugliness as a value for making engineering decisions unless all other things are equal. I think we say ugly because we can't verbal the reasons for preferring one thing over an another.) |
The old saying applies here: "If it is not broken, then don't fix it." Nothing is broken, no change is necessary. Achieving code stability is really more important than making a change to wd_start(). |
@papatience after reviewing all caller of wd_start, all place just need one argument so, the prototype can be simple as:
just like what work_s done. If out of tree usage need pass more context info, he/she can always allocate the memory as need. So here is the patch: #1565 |
If arg is the actual, single argument and NOT a pointer to an array of arguments, then this seems okay. But don't use type void *. There are problems using type void * with certain architctures (AVR I think) that was fixed by created wdparm_t. Please continue to use wdparm_t. Again, I think it is bad engineering judgement to make this change. Nothing is broken, this fixes nothing, and just adds turmoil and additional incompatibility to the documented OS internal interfaces. This is NOT responsible engineering. |
The argument was originally a uint32_t. I was changed to a union with commit b8f3bd8 because there are many architectures where sizeof(void *) < sizeof(uint32_t), MCS51, ez80, etc. So void * is insufficient for passing a uint32_t. This was changed again to the current wdparm_t form with commit 3adcae8 because there were issues in passing a union with certain compilers (SDCC at the time). So, please don't consider using void * instead of wdparm_t. You will definitely break things! |
Yes,
And if
I don't think without irq/signal/work especially irq, the system can work as expect. |
No, in general no. FAR void * is only a fatal decision if you intend to use it as a "container" to hold variables of other sizes. sizeof(uint32_t) is always 32 bits, but sizeof(void *) is variable on different MCUs from 16 to 64 bits. |
Yes, |
No. that is a false assumption. sizeof(uintptr) can be less than sizeof(uint32_t).. On most 8-bit machines, sizeof(uintptr_t) is or should be 2 bytes. On ez80 it is either 2 or 3 bytes depending on mode. For ez80, you can see: In include/stdint.h:
And in arch/z80/include/ez80/types.h:
And in limits.h:
For AVR, the size of a pointer is different on the I-space bus and on the D-space bus. As I recall, sizeof(FAR void *) is usualy 16-bits but might be as much as 24-bits). In fact sizeof(uinptr_t) < sizeof(uint32_t) is very common among the support architecures. You must not use FAR void * as a container for arbitrary values. You will break MANY architectures and we cannot permit you to do that. |
Absolutely NOT! wdparm_t was added to work around the kind of errors that you are tying to re-introduce. What you are proposing will work on most 32- and 64- bit machines but almost no where else and, hence, cannot be permitted. I will close an PR that attempts to pass watchdog paramters using FAR void * when the correct solution is already in place. What you are proposing is just wrong! If a memory storage location is allocated as a FAR void * it will be 16, 24, 32, or 64 bits wide and in many cases cannot hold a uint32_t. If it is allocated as a wdparm_t it will be 32 or 64 bits in width and is guaranteed to hold a uint32_t or a pointer in ALL cases. You proposal will not work and will break code. This problem has already been resolved and the correct type is available, please don't be stubborn and harm the system. |
On 3/19/2020 10:38 AM, Sebastian Perta wrote:
The text was updated successfully, but these errors were encountered: