シリアル通信でマルチスレッド化（同時受信データを処理したい）

Question

前提・実現したいこと

以前も似たようなご質問をさせていただきましたが、再度質問させていただきます。
以前の質問

C#でUSB接続でシリアル通信を行うWindowsアプリを作成しています。
同時に複数のデータが来た場合に対応するために、マルチスレッドに変更したいと思っております。
が、恥ずかしながらマルチスレッドの実装を行ったことがなく、ググってもイマイチだったため、
皆さまのお力を借りたいと思いました。
現状は、データが2回来て正常なデータとするため、同時に来た場合にさばけない状態となっています。
（先勝ちな状態です。）

データの一連の流れは、下記になります。
①データ受信[1回目]
②データ受信[2回目] ← ここでデータ確定
③受信データをAPI経由でDBへ格納
④戻り値を受信元へ送信

該当のソースコード

using Newtonsoft.Json;
using Newtonsoft.Json.Linq;
using System;
using System.Collections.Generic;
using System.IO;
using System.IO.Compression;
using System.IO.Ports;
using System.Net.Http;
using System.Reflection;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using System.Xml.Linq;

namespace App
{
	public partial class Form : Form
	{
		private SerialPort serialPort;

		// APIパス
		private const string API_PATH = @"http://XXXXX";

		private int			iRetry = 1;
		private int			iSeq = 0;
		private string		sBefData = string.Empty;
		private string		sData = string.Empty;
		private string		sVal1 = string.Empty;
		private string[]	sReturnVal = new string[2];
		private bool		bFirst = true;

		readonly string sPath = AppDomain.CurrentDomain.BaseDirectory;
		private string sFilePathBefore;
		string sLogFolder = "LogFile";
		private bool bSuccess = false;

		public Form()
		{
			InitializeComponent();
		}

		/// <summary>
		/// フォームロード時イベント
		/// </summary>
		/// <param name="sender"></param>
		/// <param name="e"></param>
		private void Form_Load(object sender, EventArgs e)
		{
			string[] ports = SerialPort.GetPortNames();
			foreach (string port in ports)
			{
				this.cmbSerials.Items.Add(port);
			}
			if (0 < this.cmbSerials.Items.Count)
			{
				this.cmbSerials.SelectedIndex = 0;
			}
		}

		/// <summary>
		/// 接続ボタン押下時イベント
		/// </summary>
		/// <param name="sender"></param>
		/// <param name="e"></param>
		private void btnConnect_clicked(object sender, EventArgs e)
		{
			if (this.cmbSerials.SelectedItem != null && this.serialPort == null)
			{
				// COMポートをOPEN
				serialPort = new SerialPort();
				serialPort.DataReceived += new SerialDataReceivedEventHandler(SerialPort1_DataReceived);
				serialPort.PortName  = this.cmbSerials.SelectedItem.ToString();
				serialPort.BaudRate  = 19200;
				serialPort.DataBits  = 8;
				serialPort.Parity    = Parity.None;
				serialPort.StopBits  = StopBits.One;
				serialPort.Encoding  = Encoding.UTF8;

				try
				{
					serialPort.Open();
				}
				catch (Exception ex)
				{
				}
			}
		}

		/// <summary>
		/// データ受信時イベント
		/// </summary>
		/// <param name="sender"></param>
		/// <param name="e"></param>
		private void SerialPort1_DataReceived(object sender, SerialDataReceivedEventArgs e)
		{
			if (serialPort != null && serialPort.IsOpen)
			{
				try
				{
					// 受信データを読み込む
					SerialPort sp = (SerialPort)sender;
					string sChildData = sp.ReadExisting();

					if (sChildData.Length == 2 && bFisrst)
					{
						sVal1 = sChildData;
					}
					else
					{
						sData = sChildData;
						if (bFirst) {
							return;
						}
					}

					if (!string.IsNullOrEmpty(sData) && !string.IsNullOrEmpty(sVal1))
					{
						if (0 < sData.Length) {
							if (string.IsNullOrEmpty(sBefData) || sBefData != sData)
							{
								var parameters = new Dictionary<string, string>()
								{
									// APIへ渡すデータセット
								};
								var content = new FormUrlEncodedContent(parameters);

								try {
									sReturnVal = GetResponseAPI(API_PATH, content);
									sBefData = sData;
									bSuccess = true;
								} catch {
									bSuccess = false;
								}

								if (bSuccess)
								{
									if (0 < sReturnVal.Length)
									{
										try
										{
											// 送信元へデータ送信
										}
										catch (Exception ex)
										{
										}
									}
									else
									{
										this.iRetry = 1;
									}
								}
								sData = string.Empty;
								sVal1 = string.Empty;
								sReturnVal = new string[]{ };
							}
						}
					}
				}
				catch (Exception ex)
				{
				}
			}
		}

		/// <summary>
		/// 終了ボタン押下時イベント
		/// </summary>
		/// <param name="sender"></param>
		/// <param name="e"></param>
		private void btnFinish_clicked(object sender, EventArgs e)
		{
			if (serialPort != null && serialPort.IsOpen)
			{
				serialPort.Close();
				serialPort = null;
			}
		}

		/// <summary>
		/// APIをたたいてレスポンスを取得
		/// </summary>
		private string[] GetResponseAPI(string sApiUrl, FormUrlEncodedContent sContent)
		{
			string[] sVal = new string[2];
			var task = Task.Run(() => {
				return HttpPost(sApiUrl, sContent);
			});
			string response = task.Result;
			JArray data = (JArray)JsonConvert.DeserializeObject(response);
			foreach (JObject obj in data)
			{
				JValue did = (JValue)obj["device_id"];
				JValue val = (JValue)obj["return_data"];
				sVal[0] = (string)did.Value;
				sVal[1] = (string)val.Value;
			}

			return sVal;
		}

		/// <summary>
		/// APIをPOSTする
		/// </summary>
		/// <param name="sUrl">APIのURL</param>
		/// <param name="fContent">引数をエンコードしたもの</param>
		/// <returns></returns>
		async public Task<string> HttpPost(string sUrl, FormUrlEncodedContent fContent)
		{
			var client = new HttpClient();
			var response = await client.PostAsync(sUrl, fContent);
			return await response.Content.ReadAsStringAsync();
		}
	}
}

補足情報（FW/ツールのバージョンなど）

Windows 10
Visual Studio 2017
c#／Windows Form／.Net Framewrok 4.7.1

Answer 1

マルチスレッドが解決になるのでしょうか...

いまのプログラムでは、すべてのシリアルポートの受信イベントをひとつのハンドラで受け取っていて、しかもハンドラの中で「誰から来たデータか」を判別しないまま処理しているように思えます。つまり、全部ごっちゃにしてしまっているのでうまくいっていない、とすると、それをマルチスレッドにしても何も解決しないのでは。

私の趣味的には、シリアルポート毎にハンドラを個別に持たせて、それぞれのポートはそれぞれに処理させるかな。もちろん、ひとつのハンドラで、sender毎に処理を切り替えてもいいと思いますけれど。

それと、ハンドラは送信データに対して「いつ呼ばれるかわからない」ものです。極論、1回目の送信の途中で呼ばれるかも(1バイトでも受信すればハンドラは呼ばれるかも)、逆に2回送信が完了してしまってから呼ばれるかも(いつまでに呼ばれる、という期限もありません)、そのどんな場合でも正しくデータを得て、2回送信されたことの確認ができるようにプログラムを作らなきゃいけません。
そうなっているでしょうか。

---

サンプル書いてみました。シリアル周りだけ、エラーとかは無視。
全然マルチスレッドでないのは、解決したい内容にマルチスレッドが全く寄与しないためです。

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.IO.Ports;

namespace SerialMulti
{
    class Program
    {
        static void Main(string[] args)
        {
            object lockObj = new object();
            //シリアルポート毎にインスタンスを生成する
            Connection con1 = new Connection(args[0], lockObj);
            Connection con2 = new Connection(args[1], lockObj);

            string timeStamp = DateTime.Now.ToString("mm.ss.fff");
            Console.WriteLine("Start: {0}\n", timeStamp);

            //なにかキーを押したら終了
            ConsoleKeyInfo tmp = Console.ReadKey();
        }
    }
    // シリアルポートとハンドラをクラスにまとめておく
    class Connection
    {
        SerialPort port = new SerialPort();
        char[] buf = new char[4];
        object lockObj;

        public Connection(string portName, object lockObj)
        {
            this.lockObj = lockObj;
            port.PortName = portName;
            port.BaudRate = 115200;
            port.DataReceived += Port_DataReceived;
            port.Open();
        }
        ~Connection()
        {
            port.Close();
        }
        //インスタンス毎に各ポート「専用」のハンドラとなる
        private void Port_DataReceived(object sender, SerialDataReceivedEventArgs e)
        {
            //受信文字がある限り処理する。
            //受信文字が空になって一旦ハンドラを抜けても次の文字を受信したら動作が継続できなければいけない
            while (port.BytesToRead != 0)
            {
                //直近4文字のバッファに受信する
                for (int i = 0; i < buf.Length - 1; i++)
                {
                    buf[i] = buf[i + 1];
                }
                buf[buf.Length - 1] = (char)port.ReadByte();
                //直近4文字において前半2文字と後半2文字が同じか検査
                if (buf[0] == buf[2] && buf[1] == buf[3])
                {
                    string timeStamp = DateTime.Now.ToString("mm.ss.fff");
                    //念の為排他処理にしておく
                    lock (lockObj)
                    {
                        Console.WriteLine("{0} {1}:{2}{3}", timeStamp, port.PortName, buf[0], buf[1]);
                    }
                    //送り返し
                    port.Write(String.Format("{0}{1}", buf[0], buf[1]));
                }
            }
        }

    }
}

これに、マイコンと、そのUART出力をパラに別のUSB-UART変換を噛ませてPCのポートに入力、つまり「複数のシリアル信号を全く同時に入力」した時の出力がこんな感じです。(USBを通ったりしているうちにプログラムからみて完全に同時とは言えなくなるでしょうが、まぁそこそこ)

32.45.494 COM3:BC
32.45.495 COM16:BC
32.45.529 COM3:CD
32.45.543 COM16:CD
32.45.564 COM3:DE
32.45.575 COM16:DE
32.45.599 COM3:EF
32.45.607 COM16:EF
32.45.634 COM3:FG
32.45.639 COM16:FG
32.45.669 COM3:GH
32.45.671 COM16:GH
32.45.703 COM16:AB
32.45.704 COM3:AB
32.45.739 COM3:BC
32.45.751 COM16:BC
32.45.774 COM3:CD
32.45.783 COM16:CD
32.45.809 COM3:DE
32.45.815 COM16:DE

32.45.703のあたりなど、微妙なタイミングによってハンドラが呼ばれる順番が変わったりしているのが見えますが、つまりほぼ同時に入力された信号をきちんと処理できているようです。

マイコン側テストプログラムも追記

//mbed(STM32)
#include "mbed.h"

int main(void){
    Serial pc(USBTX, USBRX);
    pc.baud(115200);
    char c='A';
    int offset=0;
    while(1){
        printf("%c%c",c+offset,c+offset+1);
        fflush(0);
        printf("%c%c",c+offset,c+offset+1);
        fflush(0);
        offset=(offset+1)%('H'-'A');// 'A'～'H'を回す
        wait(40/115.2);//送り返し時間分待ち
    }
    return 0;
}

Comments

[entaro12345]

回答ありがとうございます。

＞シリアルポート毎にハンドラを個別に持たせて…
上記は、イメージ的に下記のような実装という認識で合っていますでしょうか。
serialPort.DataReceived += (s, SerialPort1_DataReceived) =>
{　};

[thkana]

そもそもserialPortの変数が一つしかない、というところで異常なものを感じますがそれはともかく、「ハンドラ(のインスタンス)を、それぞれのシリアルポート用に用意する」ということです。

[entaro12345]

回答ありがとうございます。
理解しました。
いただいたサンプルをもとに修正しましたところいい感じに動きましたので、
経過観察したいと思います。

Answer 2

べつにマルチスレッドにしなくても、SerialPortの受信イベントで駆動していけばいいです。

現状は、データが2回来て正常なデータとするため、

これではダメです。
シリアル通信では１文字づつ受信するという場合もあるので、どのような場合でも正常に動作するように配慮する必要があります
一つのメッセージの区切り（ふつうは改行コード）をはっきり決めておいて、その区切りが来たら一つのメッセージとして処理に回す、とう言うことを行いましょう

Comments

[entaro12345]

回答ありがとうございます。

＞SerialPortの受信イベントで駆動していけばいい
というのは、具体的にどう処理を変えればよいのでしょうか？
理解できずすみません。

[y_waiwai]

複数のシリアルポートを使おうが、それぞれ個別の受信イベントを使えば、さばけない状態はありえません。
これはマルチスレッドにしようがどうしようが関係ないです。
まずは、あなたの言うさばけない状態というコードを組んでみてはどうでしょう

[entaro12345]

ありがとうございます。
現時点ソースで、すでに複数データがほぼ同時に受信した場合に、DBにAPI経由で送信できていないデータがあります。

[y_waiwai]

一つのポートで複数受信ですか？
それではマルチスレッドにしようが解決できません。

[entaro12345]

一つのシリアルポートで複数データ受信になります。
PC接続のシリアルポートを親とすると子が大量にいるイメージです。

[y_waiwai]

シリアルポートってのは１対１の通信方式です。
基本的に、そもそもの設計がわるいですね

普通そういう場合は、通信プロトコルを厳密に決めて、親からの指示で子の一つを選択し、通信を行うという形式を取り、絶対に同時の通信を行わないようにします。

[entaro12345]

なるほど。
1 対 多 はよろしくないんですね。
となると、今回のケースでいうと親と子の数が同数必要になるということですね。
勉強になります。
ありがとうございます。