カッコよいプログラムが書きたい！！

質問

組み込み系のプログラムについて質問です。

以下のプログラムは、
SWが押下される度に、KEYのモード状態を変換するプログラムです。

【質問】
　　SWが押されると『今のKEYモード』から次の
　　『新たなKEYモード』に変換されます。このとき『今のKEYモード』がわからなければ、
　　次の『新たなKEYモード』に変換することができません。
　　この『今のKEYモード』についての保存方法について質問です。

　　１、(1)の“setget_keymode関数”を使用して、この関数内で『今のKEYモード』を保存するのは、
　　 カッコいいやり方でしょうか？？
　　　　→それともこんな関数を使わず、大域変数を使用して直接値の取得/保存をおこなったほうがよい？？

　　２、(2)のstatic変数は関数内で宣言せず、関数の外で宣言したほうがよい？？
　　　　→あまり関数内のstatic変数を見たことがないためこんな質問をしました。。

　　　　『今のKEYモード』は静的な変数にする必要があります。このようなときよりリエントラントな
　　　　関数にするにはどう設計すればよいでしょうか？？

よろしくお願いいたします。

//////////////////////////////////////////////////////////////////////////
　　　　　　　　　　　　　プログラム
/////////////////////////////////////////////////////////////////////////
void main(void)
{
　　while(1){
　　　　:
　　　　:
　　keydata = get_inputkey(); // SWからのKEYデータを取得する
　　chg_keymode(keydata); // KEYデータからKEYモード変換する
　　setget_keymode(GET, &keymode); // KEYモードを取得する
　　　　:
　　　　:
　　}
}

/*
* 押されたKEYデータと、『今のKEYモード』より
* 『新たなKEYモード』に変換する。
*/
void chg_keymode(UCHAR keydata)
{
　　UCHAR keymode;

　　(1)setget_keymode(GET, &keymode)
　　if( keydata == SW1が押下 )
　　　　switch(keymode){
　　　　　　case モード1:
　　　　　　　　keymode = モード2;
　　　　　　　　break;
　　　　　　case モード2:
　　　　　　　　keymode = モード3;
　　　　　　　　break;
　　　　　　　　　:
　　　　　　　　　:
　　　　　　default :
　　　　　　　　break;
　　　　}
　　}
　　setget_keymode(SET, &keymode)
}

/*
* 『今のKEYモード』を取得/保存する関数
* SET：『今のKEYモード』を保存する。
* GET：『今のKEYモード』を取得する。
*/
(1)void setget_keymode(UCHAR setget, UCHAR *nowkeymode)
{
　　(2)static UCHAR tmpkeymode = モード1;

　　if( setget == SET ){
　　　　tmpkeymode = *nowkeymode;
　　}else if( setget == GET ){
　　　　*nowkeymode = tmpkeymode;
　　}
}

回答No.1

rabbit_cat · ベストアンサー

デザインパタンでいうSingletonパタンと思えばいいですね。
C++であれば、唯一のSingletonオブジェクトをみんなで使うっていう実装になります。

１．まあ、いいんではないでしょうか。
　大域変数を使うよりは、ずっと見通しがよいです。
　Cなんでクラスは使えないわけですが、
　setget_keymode()ていう関数がつまり、Singletonオブジェクトへの　アクセス関数になっているわけです。

２．ローカルな静的変数を使うことは別に珍しくないと思います。
　今のままでいいと思いますよ。　
　C++だと静的変数は初期化のタイミングに気をつけないといけませんが、Cなら別に気にすることもないか。

組み込みだと、見易さの問題のほかに、効率っていう問題もあると思いますが、まあ、setget_keymode()関数１個程度であれば、普通はそんなに気にすることもないとは思います。もし、効率最優先ならば、setget_keymode()は、大域変数＋マクロにする、ってことも考えられますが。

もっとも、単純にリエントラントにしたいだけなら、
tmpkeymode = *nowkeymode;
て文と、
*nowkeymode = tmpkeymode;
て文を排他制御（クリティカルセクション）にすればいいだけだと思います。ただし、keymodeをgetして、それを使って何か処理して、その後keymodeをsetするって処理をしたければ、その一連の処理全体を排他制御しないと駄目です。
というわけで、chg_keymode()を排他制御にするのが一番、手っ取り早いか。

お礼

なるほど『大域変数＋マクロ』・・
やはり大域変数にアクセスするときは何らかの手続きを踏むほうがよいのですね（この場合では、直接大域変数を使用するのではなく、マクロ関数を使用してアクセスすること）。
クリティカルセクション？？システムコールとかで制御するのですかね。。ちゃんとしたOSとか使用ししたことがなく初耳です＾＾；、今wikiでちょいと勉強しました。

ありがとうございます！

回答No.5

maku_x

No.3です。お礼に対するコメントです。
> →この理由は以下と考えて問題ないですか？？
> １、ポインタはだと関数内で値が変更される危険性があるため。
> ２、処理の流れとして、引数の値を使用し、結果を戻り値で返す。という一連の流れが成り立つため。
そうですね。普通に関数の戻り値として値を返せる場合は、ポインタを使って中身を書き換える必要は無いな、と思いましたので。

> 余談ですが、構造体を引数で渡して、構造体を返す場合は、データが多いため（構造体にもよりますが。。）すべてポインタとして渡して、それの値を変更するという形が望ましいんですか？
組み込み系のソフトでは、効率上そうすることが多いです。但し構造体渡しの場合、変更前と変更後の両方の値を比較することはよくありますから、引数として渡す領域と戻り値として結果を格納する領域両方を用意して、ポインタ渡しすることになります。

回答No.4

1839cc

まず、(2)から。
大域変数よりは、なるべく関数内 static 変数を使うべきです。

(1)
許容範囲だと思いますが、あまり綺麗なものとは思えません。
本来、関数内 static は、その関数内のみで閉じた問題を解決する際に使用できる物と考えます。
しかし、これは本来 set_keymode と get_keymode という二つの関数で実現するべきところを、関数内 static を使いたいがために無理やりまとめたように見えます。
手段が目的になっているんですね。
今回は GET/SET という二つのメソッドで済んでいますが、100種のメソッドを持つ変数に対しても同様のことをしますか？
if else が 100 コならぶ光景は異常としかいえませんし、パフォーマンスも心配になってしまいます。
（もっとも、100種も操作がある時点で異常ですし、仮にそうなっても関数テーブルを引くでしょうが）

最初、私は以下のような関数を考えてみました。

UCHAR chg_keymode(UCHAR keydata)
{
　　static UCHAR keymode = モード1;
　　if( keydata == SW1が押下 )
　　　　switch(keymode){
　　　　　　case モード1:
　　　　　　　　keymode = モード2;
　　　　　　　　break;
　　　　　　　　　:
　　　　　　　　　:
　　　　　　default :
　　　　　　　　break;
　　　　}
　　}
　　return keymode;
}

これならば、keymode は関数内で閉じているように見えますし、戻り値を利用することで main からも GET を利用する必要がなくなります。
しかし、こうしてしまうと今度はその他の場所で GET/SET することが出来なくなります（必要なければそのほうがよいが）。
やはり、単にこの変数には GET/SET 以外の操作が必要だったとしか思えません。
この変数は関連する複数の関数で共有されるべき変数と考えるのが自然ではないでしょうか。
C++ であれば、複数のメソッドから操作されるため、メンバ変数にするべき変数だと思います。
ところが、それをクラス変数、もしくは関数内変数にしてしまっているような違和感を覚えます。
確かにスコープはより限定されますが、不自然ですよね。
たとえ、Singleton だったとしても。

ということで、悪くはないけど、カッコイイとは思えないという感じですね。
関連する関数のみをひとつの C ファイルにまとめ、そこでファイルローカルな変数として作成するほうが自然なのではないでしょうか。

関数内変数にするとしても、不自然さを隠すために set_keymod と get_keymod の二つのマクロ関数でラップし、引数 setget は隠したほうがよいでしょう。
chg_keymode も戻り値を返したほうがよさそうです。

＃ ちなみに、＃２さんの方法がいちばん好みです

お礼

補足 この例題プログラムだけでは全く分かりませんが、keymodeはその他の場所で GET/SETすると考えています。

>関連する関数のみをひとつの C ファイルにまとめ、そこでファイルローカルな変数として作成するほうが自然なのではないでしょうか。
→そのとおりだと思います。勉強になります！

>set_keymod と get_keymod の二つのマクロ関数でラップ
→これもまたまた、そのとおりだと思います！勉強になります！

ありがとうございます！

回答No.3

maku_x

1. それで良いと思います。おもむろにグローバル変数を使うのは、コードが読みにくくなるので避けるべきです。できれば set と get を分けたいところですが、それはマクロでごまかしても良いでしょう。

2. その関数の中だけでしか static 変数を使わないのであれば、この方が変数のスコープが限定できるので、良いと思います。

3. 呼び出し階層の制限がつきますが、tmpkeymode を配列で宣言し、呼び出し階層を保持する static 変数を宣言し、呼び出し階層ごとに keymode を保存するようにすれば良いでしょう。あと、使えるならセマフォを使うとか。

※ 余談になりますが、void setget_keymode(UCHAR setget, UCHAR *nowkeymode) は、UCHAR setget_keymode(UCHAR setget, UCHAR nowkeymode) として、現在の KEY モードを戻り値にするほうが良いかと。

お礼

余談のところで質問です！

>余談になりますが、void setget_keymode(UCHAR setget, UCHAR *nowkeymode) は、
>UCHAR setget_keymode(UCHAR setget, UCHAR nowkeymode) として、現在の KEY モードを戻り値にするほうが良いかと。

→この理由は以下と考えて問題ないですか？？
１、ポインタはだと関数内で値が変更される危険性があるため。
２、処理の流れとして、引数の値を使用し、結果を戻り値で返す。という一連の流れが成り立つため。

※余談ですが、構造体を引数で渡して、構造体を返す場合は、データが多いため（構造体にもよりますが。。）すべてポインタとして渡して、それの値を変更するという形が望ましいんですか？

回答No.2

MrBan

# 典型的な有限状態マシン？

>『今のKEYモード』は静的な変数にする必要があります。
これを何とかするのがリエントラントにする近道では？

外部要件がわからないので無理なのかも知れませんが、
制約がなければ個人的にはこうするかな。(静的には置かない)
(current_modeにはエラーというモードも等価に盛り込んでおく前提)
for(;;)
{
　　UCHAR　current_mode = INITIAL_MODE;
　　　:
　　　:
　　keydata = get_inputkey(); // SWからのKEYデータを取得する
　　current_mode = get_nextkeymode(current_mode, keydata); // KEYデータから変換したモードを返す
　　　　:
　　}
}

お礼

すいません。現在のプログラムではmain関数内で、ＫＥＹ処理のみの処理を行っていますが、このＫＥＹ処理は１つのタスクとイメージしてください。
この場合であれば、KEYタスクを終了した後に、再びmain関数に戻って処理をするイメージです。とすると、この『今のKEYモード』は必ず静的に処理する必要があると思います。

わかりにくてすいませんです・・