elektron-bbs commented 1 year ago

Please check if the PR fulfills these requirements
[ ] Tests for the changes have been added / modified (needed for for bug fixes / features)
[x] commandref has been added / updated (needed for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
[ ] Bugfix (please link issue)
[ ] Feature enhancement
[x] Documentation update
[ ] Unittest enhancement
[ ] other
What is the current behavior?
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?) no
Other information:

codecov[bot] commented 1 year ago

Codecov Report

Merging #1151 (2a44ae6) into master (8c85ff3) will decrease coverage by 0.02%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1151      +/-   ##
==========================================
- Coverage   67.47%   67.45%   -0.02%     
==========================================
  Files         133      135       +2     
  Lines        9778     9812      +34     
  Branches     1570     1570              
==========================================
+ Hits         6598     6619      +21     
- Misses       1885     1898      +13     
  Partials     1295     1295

Flag	Coverage Δ
fhem	`57.05% <100.00%> (+0.01%)`	:arrow_up:
modules	`67.45% <100.00%> (-0.02%)`	:arrow_down:
perl	`90.33% <ø> (ø)`
unittests	`67.45% <100.00%> (-0.02%)`	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
FHEM/41_OREGON.pm	`28.85% <ø> (ø)`
FHEM/00_SIGNALduino.pm	`63.90% <100.00%> (ø)`
FHEM/10_SD_Rojaflex.pm	`65.99% <0.00%> (-4.46%)`	:arrow_down:
t/FHEM/10_SD_Rojaflex/09_parseData.t	`94.11% <0.00%> (ø)`
t/FHEM/14_SD_WS/09_parseData.t	`94.11% <0.00%> (ø)`

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

elektron-bbs commented 1 year ago

Wie sollte es auch anders sein: codecov/patch — 0.00% of diff hit (target 58.98%)... Schreibst du was spezielles, oder gleich ein # uncoverable branch true?

sidey79 commented 1 year ago

Das Ergebnis ließe sich leicht testen. Wieso das ein Bug ist verstehe ich allerdings noch nicht. Damit wird doch das Standard Verhalten verändert.

elektron-bbs commented 1 year ago

default in Parse Zeile 901:

my $longids = 1;

Angelegt ist bereits: THR128_0a_1

Beliebiges Device in Attribut longids schreiben:

attr sduinoEasy434 longids SD_WS_33_T

Es wird eine zusätzliches Device ohne longid angelegt:

2023.01.17 09:48:00.225 3: OREGON: Unknown device THR128_1, please define it
2023.01.17 09:48:00.245 2: autocreate: define THR128_1 OREGON THR128_1
2023.01.17 09:48:00.262 2: autocreate: define FileLog_THR128_1 FileLog ./log/THR128_1-%Y-%m.log THR128_1
2023.01.17 09:48:00.264 2: autocreate: define SVG_THR128_1 SVG FileLog_THR128_1:temp4:CURRENT

weil keine der Bedingungen mehr zutrifft:

# Test if to use longid for device type
sub OREGON_use_longid {
  my ($longids,$dev_type) = @_;

  return 0 if ($longids eq "");
  return 0 if ($longids eq "NONE");
  return 0 if ($longids eq "0");

  return 1 if ($longids eq "1");
  return 1 if ($longids eq "ALL");

  return 1 if(",$longids," =~ m/,$dev_type,/);

  return 0;
}

Das passiert nicht, wenn wir default (1) zurück geben.

sidey79 commented 1 year ago

Das passiert nicht, wenn wir default (1) zurück geben.

Ja ok, das habe ich verstanden, weil der Standard bei ungesetztem Attribut im Oregon Modul eine 1 ist.

Unsere Commandref (DE/EN) ist sich hier aktuell auch nicht einig

  # Keine langen IDs verwenden (Default Einstellung):
      attr sduino longids 0
      # Immer lange IDs verwenden:
      attr sduino longids 1

    Default is to not to use long IDs for all devices.
    <br><br>
    Examples:<PRE>
      # Do not use any long IDs for any devices:
      attr sduino longids 0
      # Use any long IDs for all devices (this is default):
      attr sduino longids 1

Wo man sich aber einig ist Durch Komma getrennte Liste von Device-Typen für Empfang von langen IDs mit dem SIGNALduino.

Demnach ist es ja richtig, dass keine longid für das OREGON Modul verwendet wird, wenn SD_WS_33_T festgelegt wird. Wie sonst, sollte man denn longids für Orgon ausschalten, wenn man es für andere Module verwendet?

elektron-bbs commented 1 year ago

Naja, das ist etwas verzwickt, weil default longids bei OREGON 1 ist und bei SD_WS, SD_WS07, HIDEKI usw. halt 0. Wenn wir default bei OREGON ändern, verärgern wir die User. Abschalten der lonidds bei OREGON funktioniert aktuell gar nicht, sobald man bei einem anderen Device longids haben möchte.

Mich störte halt nur, das es mir einen zweiten OREGON anlegt, sobald ich bei anderen Geräten longids brauche.

sidey79 commented 1 year ago

Das ist wirklich verzwickt ja.

Wenn ich longids aber nur für SD_WS aktiviere, dann ist es richtig, dass der Oregon Sensor danach ohne longids angelegt wird. So ist es dann ja gewollt. Oregon habe ich selbst ohne longids Nutzung in Betrieb, das geht durchaus zu deaktivieren.

HomeAutoUser commented 1 year ago

Das ist wirklich verzwickt ja.

Was wäre dein Vorschlag diese Verzwicktheit etwas klarer zu gestalten?

Den Vorschlag von @elektron-bbs mit der Änderung finde ich gut. Sollten wir dies nicht machen, so bitte ausgiebig die Commandref anpassen. Unwissende User würden definitiv darüber stolpern und Fragen generieren.

sidey79 commented 1 year ago

@HomeAutoUser

Ich finde die Anpassung nicht gut, denn das Attribut funktioniert dann nicht mehr. Also wenn ich longids für Oregon abschalten und für SD_WS einschalten möchte, wie mache ich das dann?

HomeAutoUser commented 1 year ago

@HomeAutoUser

Ich finde die Anpassung nicht gut, denn das Attribut funktioniert dann nicht mehr. Also wenn ich longids für Oregon abschalten und für SD_WS einschalten möchte, wie mache ich das dann?

Wenn du LongId´s für Oregon ausschaltest und für SD_WS07 ein, dann Ergänzt du im Attribut LongID´s Bsp: SD_WS07_T

Die jetzige Handhabung ist ja genau umgekehrt wenn ich das Verhalten richtig verfolge. Das ist nirgends dokumentiert und wenn jemand das Attribut bisher nicht verwendete aber dann Bsp. für SD_WS07 setzt, so kommt der Oregon auf einmal "doppelt". Einmal mit Long und einmal mit Short ID. @elektron-bbs korrigiere mich bitte wenn das verkehrt war. Somit stolpert man automatisch, so auch wir beide und suchten erstmal den "Wolf".

Sollte keine Anpassung erfolgen, so bitte ich im Sinne von den anderen Usern, das ausführlich in der Commandref zu benennen.

Auszug aus Commandref https://github.com/RFD-FHEM/RFFHEM/blob/master/FHEM/00_SIGNALduino.pm#L5338:L5349

sidey79 commented 1 year ago

Wenn du LongId´s für Oregon ausschaltest und für SD_WS07 ein, dann Ergänzt du im Attribut LongID´s Bsp: SD_WS07_T

Ich glaube so funktioniert es heute, aber mit der Anpassung nicht mehr. Vielleicht sollte ich ein paar Tests schreiben, damit wir Gewissheit erlangen.

elektron-bbs commented 1 year ago

Ich denke, @sidey79 hat Recht. Longids für OREGON ausschalten und für andere einschalten, geht mit dem Patch nicht mehr. Ich ziehe den PR zurück, offensichtlich können die User damit leben :-)

HomeAutoUser commented 1 year ago

Vielleicht sollte ich ein paar Tests schreiben, damit wir Gewissheit erlangen.

Da kannst du ja alles durchspielen :-)

Ich ziehe den PR zurück, offensichtlich können die User damit leben :-)

Das ist kein Freischein, um die mangelnde Commandref unter den Tisch zu kehren :-D

elektron-bbs commented 1 year ago

Ich habe jetzt die ursprüngliche Änderung zurück gesetzt und stattdessen die commandref angepasst.

elektron-bbs commented 1 year ago

Habe es angepasst.

RFD-FHEM / RFFHEM

correction commandref longids #1151

Codecov Report