Re: [Xenomai-core] [PATCH] solo/vxWorks: add rngLib (with testsuite)
Am Sonntag 26 Oktober 2008 17.02:08 schrieb Philippe Gerum:
Niklaus Giger wrote:
Hi
I just found the time to prepare a patch for a substitute for the vxWorks
rngLib library I have written quite some time ago.
..
Thank you for your review. Attached a corrected version of my patch.
But there a some more points, which I would like to discuss.
Fair enough, but we would have to pin all VxWorks tasks to the same CPU on
SMP systems; otherwise, out-of-order memory accesses would kill that code.
Can you do it this?
I am not familiar with this kind of problem as I am still running single
processor boards. But I am quite sure that our next board will have two (or
more) processore cores sitting on it. Though I have quite an interest in
providing a correct solution.
I am not familiar with vxWorks-SMP and therefore I am not sure whether it is a
good idea to pin all vxWorks tasks to one CPU or whether the user should be
able to select the tasks he wants. E.g. you might want only the reader and the
writer of a ring or list on one CPU and other ones to other CPU(s).
..
vxworks/Makefile.in | 18 -
vxworks/rngLib.c| 152
+++ vxworks/rngLib.h|
34 ++
Something must be missing here. vxworks/rngLib.h should contain internal
definitions, used by the ring buffer implementation code, and that is
correct in your code. However, include/vxworks/rngLib.h should provide the
interface to applications; it seems we are missing such a file.
Sorry, I forgot to do git add include/vxworks/rngLib.h, which is attached now
and which provides the (opaque) interface to the vxWorks users.
Since we don't want to export the innards of a ring buffer implementation
to applications, the interface header should only define the opaque ring
buffer handle type (i.e. typedef intptr_t RING_ID), and the RNG routine
prototypes.
..
Please use the linux-c coding style, basically KR + preposterously large
indent tabs (8 chars)
Thank you for code snippets for emacs and ident (did put it into my notes as
it is not the first time I have to deal with such problems).
Emacs can be taught this way:
(defun linux-c-mode ()
C mode with adjusted defaults for use with Xenomai.
(interactive)
(c-mode)
(c-set-style KR)
(setq tab-width 8)
(setq indent-tabs-mode t)
(setq c-basic-offset 8))
Indent will reformat almost properly with (-T should be used to teach
indent about the known non-standard types as well):
$ indent -npro -kr -i8 -ts8 -sob -l80 -ss -ncs -cp1
+ ring_mem = malloc( sizeof(RING_DESCRIPTOR*) + nbytes);
The sizeof() argument is wrong. Out-of-bound memory writes are knocking at
the door.
Thanks for catching this error. The following code should be correct.
ring_mem = xnmalloc(sizeof(RING_DESCRIPTOR) + nbytes + 1);
I need 1 additional byte to be able to use only the read/write-position. This
allows that the reader only modifies the readPosition and ther writer only
the writePosition.
Also, please use xnmalloc(). This call will pull memory from the allocator
selected at built time for the SOLO stack, either TLSF, or standard
malloc(). --enable-malloc is false by default (i.e. TLSF enabled).
+ if ( ring_mem == 0) { errno = errnoSet(S_memLib_NOT_ENOUGH_MEMORY);
return 0; } +
Ack.
Please compare pointers to NULL (and return null pointers the same way),
that gives a better hint about the pointerness of the data involved.
Ack.
+ ring = (RING_DESCRIPTOR *) ring_mem;
Cast is useless; let's remove visual clutter as much as we can. Actually,
ring_mem is useless as well; you could optimally work only using the ring
variable directly. Reducing the number of variable hops allows people to
work late at night with half a brain.
Ack.
+ ring-magic = WIND_RING_MAGIC;
+ ring-bufSize = nbytes;
+ ring-readPos = 0;
+ ring-writePos = 0;
Nitpicking alert: line-feed please. It is visually easier to locate the
main return value.
Ack.
+ return (RING_ID) ring_mem;
+}
+
+
+void rngDelete(RING_ID ring_id)
+{
+ RING_DESCRIPTOR *ring = (RING_DESCRIPTOR *) ring_id;
+ if (ring-magic != WIND_RING_MAGIC) return;
Ack.
KR
+ ring-magic = 0;
+ free(ring);
xnfree() is the converse call for xnmalloc().
Ack.
..
linefeed please. Splitting declarations and pure executable statements
makes it easier for the eyes as well.
Ack.
+ if (ring-magic != WIND_RING_MAGIC) return -ENOSYS;
Mm, I guess VxWorks is not even checking there, right? If it does not, then
I would rather raise an assertion when a bad magic is detected, that would
only trigger with --enable-debug/--enable-assert.
It is true, that vxWorks does not use a magic.
What is your prefered way to raise an exception?
+ for (j=0; j maxbytes; j++)
+ {
+ if ((ring-readPos) % (ring-bufSize + 1) == savedWritePos)
ring-readPos should never be out of bounds anyway. Do we
Re: [Xenomai-core] [PATCH] solo/vxWorks: add rngLib (with testsuite)
Niklaus Giger wrote:
Hi
I just found the time to prepare a patch for a substitute for the vxWorks
rngLib library I have written quite some time ago.
The remark from the vxWorks description applies here too
In particular, ring buffers by themselves provide no task synchronization or
mutual exclusion.
(http://www.slac.stanford.edu/exp/glast/flight/sw/vxdocs/vxworks/ref/rngLib.html)
Therefore this library is only good were there is exactly one writer an one
reader
envolved.
Fair enough, but we would have to pin all VxWorks tasks to the same CPU on SMP
systems; otherwise, out-of-order memory accesses would kill that code.
It would be nice if it could find a place in the xenomai-solo. I am willing to
address any criticism/nitpicking needed to get it into a good shape.
Best regards
Signed-off-by: Niklaus Giger [EMAIL PROTECTED]
---
include/vxworks/Makefile.am |1 +
include/vxworks/Makefile.in |1 +
vxworks/Makefile.am |2 +
vxworks/Makefile.in | 18 -
vxworks/rngLib.c| 152
+++
vxworks/rngLib.h| 34 ++
Something must be missing here. vxworks/rngLib.h should contain internal
definitions, used by the ring buffer implementation code, and that is correct in
your code. However, include/vxworks/rngLib.h should provide the interface to
applications; it seems we are missing such a file.
Since we don't want to export the innards of a ring buffer implementation to
applications, the interface header should only define the opaque ring buffer
handle type (i.e. typedef intptr_t RING_ID), and the RNG routine prototypes.
vxworks/testsuite/Makefile |2 +-
vxworks/testsuite/rng-1.c | 118 +
8 files changed, 323 insertions(+), 5 deletions(-)
create mode 100644 vxworks/rngLib.c
create mode 100644 vxworks/rngLib.h
create mode 100644 vxworks/testsuite/rng-1.c
[snip]
diff --git a/vxworks/rngLib.c b/vxworks/rngLib.c
new file mode 100644
index 000..d033db9
--- /dev/null
+++ b/vxworks/rngLib.c
@@ -0,0 +1,152 @@
+/*
+ * Copyright (C) 2008 Niklaus Giger [EMAIL PROTECTED].
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ */
+
+#include stdlib.h
+#include vxworks/errnoLib.h
+#include rngLib.h
+
+#define WIND_RING_MAGIC 0x5432affe
+
+RING_ID rngCreate(int nbytes)
+{
+ RING_DESCRIPTOR *ring;
+ void *ring_mem;
+
+ if ( nbytes=0) { errnoSet(S_memLib_NOT_ENOUGH_MEMORY); return 0; }
Please use the linux-c coding style, basically KR + preposterously large indent
tabs (8 chars)
Emacs can be taught this way:
(defun linux-c-mode ()
C mode with adjusted defaults for use with Xenomai.
(interactive)
(c-mode)
(c-set-style KR)
(setq tab-width 8)
(setq indent-tabs-mode t)
(setq c-basic-offset 8))
Indent will reformat almost properly with (-T should be used to teach indent
about the known non-standard types as well):
$ indent -npro -kr -i8 -ts8 -sob -l80 -ss -ncs -cp1
+ ring_mem = malloc( sizeof(RING_DESCRIPTOR*) + nbytes);
The sizeof() argument is wrong. Out-of-bound memory writes are knocking at the
door.
Also, please use xnmalloc(). This call will pull memory from the allocator
selected at built time for the SOLO stack, either TLSF, or standard malloc().
--enable-malloc is false by default (i.e. TLSF enabled).
+ if ( ring_mem == 0) { errno = errnoSet(S_memLib_NOT_ENOUGH_MEMORY);
return 0; }
+
Please compare pointers to NULL (and return null pointers the same way), that
gives a better hint about the pointerness of the data involved.
+ ring = (RING_DESCRIPTOR *) ring_mem;
Cast is useless; let's remove visual clutter as much as we can. Actually,
ring_mem is useless as well; you could optimally work only using the ring
variable directly. Reducing the number of variable hops allows people to work
late at night with half a brain.
+ ring-magic = WIND_RING_MAGIC;
+ ring-bufSize = nbytes;
+ ring-readPos = 0;
+ ring-writePos = 0;
Nitpicking alert: line-feed please. It is visually easier to locate the main
return value.
+ return (RING_ID) ring_mem;
+}
+
+
+void rngDelete(RING_ID ring_id)
+{
+ RING_DESCRIPTOR *ring = (RING_DESCRIPTOR *)
