15

Data Acquisition You asked: Is there a smarter way to accomplish the result of my code above? So, I have another suggestion, to work on top of the suggestions by Heslacher: Eliminate the BitConverter work. This is easily done with boolean operators (most specifically the shift operators). short value1 = (short)((short)encoded[1] << 8 | encoded[2]...


12

Character based serial communications are classically difficult to program in theory.  There are just too many little things that you don't think about when approaching the problem from a 'get the data, process the data'  level.  E.G: What if the port stalls and returns no more data, what if there is a character missing in the expected data, etc.  In many ...


9

Have you actually tested your code? From the first look - it wont even work as intended: This code is meaningless if (received.Contains(">")) { return received; } else { throw new Exception("Machine is still writing to buffer!"); } ReadLine call does not return EOL symbol, so there ...


9

You should just include <cstddef> in order to use std::size_t. Including <cstring> seems unnecessary if you're not actually going to utilize anything else. Since this is C++11, you can now use strongly-typed enums over plain ones. One difference is that plain enums are implicitly cast to int, whereas strongly-typed ones must be explicitly cast ...


8

The code you posted contains no error-recovery at all. It's a bit off-topic on this site to ask how to implement a new feature (error-recovery); but I'll try. It's not clear what your communication protocol looks like. It might be: A continuous stream of bytes, to be split into packets of 10 10-byte packets, with a measurable delay between packets, no ...


8

I have a number of questions that you might find useful to answer for yourself with the idea of improving your code. Are read and send really const? It would be a most unusual implementation of a serial port class that didn't actually modify internal state when doing a read or send operation. These should probably not be declared const, including ...


8

Another suggestion, to go on top of Heslacher's: Make the (short)((short)inputBuffer[... a method. [MethodImpl(MethodImplOptions.AggressiveInlining)] public static short BytesToShort(byte a, byte b) { return (short)(a << 8 | b); } The MethodImplOptions.AggressiveInlining enumeration value tells the C# compiler to try to inline the method as much ...


8

I would recommend your outputs be flops. It would be a cleaner output signal and easier to do timing analyses. It takes time (few nano seconds) to calculate the tx value after the clock with combinational logic. During that time the intermediate values are being transmitted and causing noise. This noise will can get worse from resistance an capacitance in ...


7

You only need to get the bytes if line[0]=='F' so you should move it inside the if block. If you only need to get these 3 Int16 values, IMHO using Array.Copy, Array.Reverse is a little bit too much. while(SerialPort.BytesToRead > 50){ string line = SerialPort.ReadLine(); if (line[0]=='F'){ byte[] encoded = enc.GetBytes(line); ...


6

The .NET serial port is notorious for making it easy to write horrible code :( As far as reviewing yours, I can't get past the usage of Thread.Sleep(). It's wrong, irredeemably broken, because the sleep starts when the data is appended to the kernel buffer, not when it gets transmitted. Actual time of transmit is affected by how much data was already in ...


6

Since your object owns a resource (SerialPort) which implements the IDisposable interface, your class must also implement it. Also, for repeated string concatenations, the StringBuilder class gives much better performance. I've cleaned up a bit and came up with the following: namespace MedSerialPort { using System; using System.IO.Ports; using ...


6

By extracting the retrieval of the different sensordata to separate methods this whole code block would be more readable and easier to maintain. By using a for loop for reading the single values the whole reading of the sensor data can be done inside a single method. By replacing the big if statements like so int? numberOfSensorData; int ...


6

There are a number of strange things going on here... First, you have this: //Buffer with data byte[] data = HexStringToByteArray(mensage); //Handle data comport.Read(data, 0, data.Length); The first line takes the mensage (message?) and turns it into a byte array, but then you immediately overwrite the data by reading into the same buffer. In essence, ...


6

It is kind of hard to say without seeing the whole code but at a glance the whole thing seems kind of pointless to me. You're not making it object oriented but rather just adding your own "flavor" API over libusb. I can tell this by the fact that I see a DevHandlePtr deviceHandle being passed around just about everywhere. If you want to actually make a ...


5

Here are some things that may help you improve the quality of your code. Don't handle message parsing in an interrupt You don't explicitly say it, but from the context, it appears that you're attempting to parse a serially received message in an interrupt handler. This is usually not a good idea. An interrupt handler should be fast and do the minimum ...


5

First off, you should probably allow a default ComName to be passed into the constructor, and the default should probably be null instead of an empty string (unless you have good reason to do otherwise). function Serial(name) { this.ComName = name !== undefined ? name : null; } Next, getPortName is strange. You set it on Serial.prototype, which ...


5

Finding Performance Problems It currently takes 16 seconds from pressing run in Eclipse to something appearing on my screen. The first step to fixing this is to find out what is causing the delay. My first assumption would be that it is the hardware on the other end of the connection, but who knows. To figure this out, you can do two things: Mock the ...


5

There are definitely some things you can do to make this code more clear. Eliminate global variables where practical Having routines dependent on global variables makes it that much more difficult to understand the logic and introduces many opportunities for error. Eliminating global variables where practical is always a good idea, whether programming for ...


4

On your questions: There are some oddities in your code, for example, the use of non-blocking sockets. Detecting disconnected clients is difficult. I'm not that familiar with UNIX-domain sockets, but I imagine they are much the same as Internet-domain sockets. You probably need either to set up periodic keep-alive messages within whatever protocol you ...


4

Here are some ideas that may be useful to you. Decide what's most important Since the 8051, including your AT89S52 variant, has few resources, you'll have to make some hard decisions on how to use them. If you're already using the single serial port for some other purpose and it's fixed for some reason (e.g. can't respin boards), then perhaps it's out of ...


4

Unfortunately your code is incomplete and can't be run in this state. Still I want to recommend some improvements to this snippets. My first impression from this code is that it looks like plain C, not C++ (besides the use of std::equal). To pass your Message and compare it you are using plain char arrays. With this approach you have to also pass the ...


3

Style init_serial is quite hard to follow. It took me a while to realize that code in a nested try-except is not a retry. Factor out the identical code into a function. Also, there is no need to return a success status in a separate boolean: returning None is just fine, in a boolean context it is False: For example: def init_serial(device_count = 10): ...


3

I would modify your serial reading function in a few ways, firstly by changing its name to reflect what it does. I would also move the accumulation buffer into the function, making it and its position counter static (which means they stick around without losing their value between calls) . Also define the details of the packet in #defines. More extensive ...


3

You shouldn't need to call usleep() to throttle the loop. If you need usleep() prevent the loop from consuming 100% of the CPU, then something is wrong, since select() is supposed to be the gatekeeper that lets the loop continue when input is available to be processed. Perhaps you are failing to drain the socket or serial port completely in your input ...


3

The final version, in case someone was interested. main.c int local_socket; int serial_fd; int max_fd; fd_set input; fd_set tmp_input; char *serial_output_buffer; serial_output_buffer=malloc(11 * sizeof(char)); serial_fd=open_port(); local_socket=open_local_socket(); FD_ZERO(&input)...


3

I'll start by breaking the code down into different files. There are a many #define and const int here. If I am correct that will definitely grow with your project. Having all the constants in a different file is always a good idea if you are considering making it even moderately big. So to your question Should making a library be considered? I would ...


3

Not sure if still relevant, but here you go: Consider refactoring if (this.disposed) { } into a method like ThrowIfDisposed(). If you ever want to change the dispose method or add logging or whatnot then you have to change only one place. Code duplication should be avoided even for trivial things like that. You swallow exceptions in Hook and Unhook and ...


3

Yes I think you do break SRP. Currently your class is pretty much untestable due to the hard-coded implicit dependency on SerialPort which is going to be very hard to mock out. You should definitely remove the connection handling (open, close, send, receive) into a separate interface and inject that instead. A few more things I noted: Your two Send ...


3

_port.Encoding.GetBytes(_port.ReadExisting()) If you want to work with byte[], then your data should never appear as string, even temporarily, because some byte sequences are likely going to be invalid in your encoding. I believe you should be able to use the byte[] overload of Read() for this. (Though switching to byte[] and async methods of the ...


3

C++ Please use some form of consistent indentation, currently it's all over the place. For example: if (command.data1 & FORWARD_DIRECTION){ goForward(); dbg_print("Drive Forward ..."); }else if (command.data1 & BACKWARD_DIRECTION){ goBackward(); dbg_print("Drive Backward ..."); } This is so much easier to read if the ...


Only top voted, non community-wiki answers of a minimum length are eligible