Add multi-message feature, for handling multiple sensor data in one message #1515

cpetra · 2022-02-06T14:10:46Z

The "blob" feature acts like a special "sensor" type (V_MULTI_MESSAGE). The gateway will reconstruct different messages from this special object and will send them to the controller.
This could be improved by adding the possibility to automatically do a send when the message is full.

mfalkvidd · 2022-02-06T19:21:17Z

Nice work!

I checked ram and flash usage. This feature does not change flash and ram usage when unused, and lowers flash and ram usage when used.

Development branch, sketch without USE_BLOB:
Sketch uses 7606 bytes (24%) of program storage space. Maximum is 30720 bytes.
Global variables use 445 bytes (21%) of dynamic memory, leaving 1603 bytes for local variables. Maximum is 2048 bytes.

cpetra branch, without USE_BLOB
Sketch uses 7606 bytes (24%) of program storage space. Maximum is 30720 bytes.
Global variables use 445 bytes (21%) of dynamic memory, leaving 1603 bytes for local variables. Maximum is 2048 bytes.

cpetra branch, with USE_BLOB
Sketch uses 7494 bytes (24%) of program storage space. Maximum is 30720 bytes.
Global variables use 412 bytes (20%) of dynamic memory, leaving 1636 bytes for local variables. Maximum is 2048 bytes.

The build system is out of order. I don't know why, but I have sent a question to the rest of the core team.

mfalkvidd · 2022-02-06T19:36:50Z

Link to forum thread: https://forum.mysensors.org/topic/11893/multiple-sensor-data-in-one-message

core/MyMessage.h

virtual-maker · 2022-02-06T22:42:45Z

@cpetra Cool new feature! Thank you for sharing it!

I looked into some of your commits and would like to add some comments to minor issues. For this it would be very useful for a code review via the GitHub web UI to squash your 6 commits into a single commit and force push it again. Then I see all your adds and changes to a file at once.

And while doing this it would be perfect to give this single commit a descriptive commit message with reference to the associated issue ID e.g. "Add multi message support (#1234)".

Please let me know, to create this issue (feature request).

cpetra · 2022-02-06T23:05:49Z

I agree with this, it's a bit messy. I'm not sure how this is handled by github. I know on gitlab one can "squash commits" and change the commit message for a merge request and I expected this would be done by the one doing the actual merge.

virtual-maker · 2022-02-06T23:27:12Z

Have your checked this MySensors Contribution Guideline?

Unfortunately squash of multiple commits is not mentioned there. But basically you should squash the commits on your local Git repo and force push it to your MySensors GitHub forked repo. Then your change will go automatically into this MR.

Regarding squash by use of the Git CLI I found this helpful: Squash commits into one with Git

If this is new to you, I highly recommend to make a copy of your local repository before!

mfalkvidd · 2022-02-07T05:18:36Z

core/MyBlob.cpp

+	} __attribute__((packed));
+} __attribute__((packed)) blob;
+
+MyBlob::~MyBlob()


Perhaps renaming MyBlob to MyMultiMessage, to keep naming more similar to MyMessage?

I was kind of afraid of that :) I am actually quite fond of the "Blob" naming, in my programming history it always meant a conglomerate of different type of data. I also find it fun to use and easy to remember. There is, now, the V_MULTI_MESSAGE sensor type that makes the connection for the unaware.

cpetra · 2022-02-07T05:29:24Z

Have your checked this MySensors Contribution Guideline?

Unfortunately squash of multiple commits is not mentioned there. But basically you should squash the commits on your local Git repo and force push it to your MySensors GitHub forked repo. Then your change will go automatically into this MR.

Regarding squash by use of the Git CLI I found this helpful: Squash commits into one with Git

If this is new to you, I highly recommend to make a copy of your local repository before!

Thanks for pointing that out! Yes, I have read it, but got side-tracked and forgot about the "Ammending" part. I have now force-pushed again the changes into one commit.

mfalkvidd · 2022-02-07T08:31:43Z

I don't know why the build bot is unable to post the result automatically, but here is the butler report:

Greetings! Here is my evaluation of your pull request:

No subjects are of invalid size - great!

No subjects with leading lower case characters - great!

Commit subjects with trailing periods:
Added blob feature for sending multiple sensor data in one message.

No body lines are too wide - great!

No keywords are missing in keywords.txt - great!

No keywords in code that don't exist in keywords.txt - great!

No keywords in code that don't have Doxygen comments and aren't blacklisted in keywords.txt - great!

No lines in keywords.txt using spaces instead of TAB (the Arduino IDE doesn't support space) - great!

No occurences of the deprecated boolean data type - great!

I am afraid there are some issues with your commit messages and/or use of keywords.
I highly recommend reading this guide for tips on how to write a good commit message.
More specifically, MySensors have some code contribution guidelines that I am afraid all contributers need to follow.

I can help guide you in how to change the commit message for a single-commit pull request:
git checkout <your_branch>
git commit --amend
git push origin HEAD:<your_branch_on_github> -f

To change the commit messages for a multiple-commit pull request:
git checkout <your_branch>
git rebase -i <sha_of_parent_to_the_earliest_commit_you_want_to_change>
Replace "pick" with "r" or "reword" on the commits you need to change message for
git push origin HEAD:<your_branch_on_github> -f

I am afraid your coding style is not entirely in line with the MySensors prefered style.
A mail with a patch has been sent to you that, if applied to your PR, will make it follow the MySensors coding standards.
You can apply the patch using:
git apply restyling.patch

If you have any questions, please first read the code contribution guidelines.
If you disagree to this, please discuss it in the GitHub pull request thread.

Yours sincerely, The Butler, serving the MySensors community

cpetra · 2022-02-07T09:01:55Z

@mfalkvidd

Commit subjects with trailing periods:
Added blob feature for sending multiple sensor data in one message.
Is this still valid after the amend/forced push? I assume that should have cleared the bad history

mfalkvidd · 2022-02-07T11:57:25Z

You're probably right.

Here is the new feedback from the build system:

Butler report

Greetings! Here is my evaluation of your pull request:

No subjects are of invalid size - great!

No subjects with leading lower case characters - great!

No subjects with trailing periods - great!

No body lines are too wide - great!

No keywords are missing in keywords.txt - great!

No keywords in code that don't exist in keywords.txt - great!

No keywords in code that don't have Doxygen comments and aren't blacklisted in keywords.txt - great!

No lines in keywords.txt using spaces instead of TAB (the Arduino IDE doesn't support space) - great!

No occurences of the deprecated boolean data type - great!

This commit is meeting the coding standards, well done!

Yours sincerely, The Butler, serving the MySensors community

Documentation

MyBlob.h:23, Doxygen, Priority: Normal

missing title after \defgroup MyBlob

MyBlob.h:28, Doxygen, Priority: Normal

explicit link request to 'define' could not be resolved

MyBlob.h:108, Doxygen, Priority: Normal

Member MyBlob(MyMessage *msg) (function) of class MyBlob is not documented.

MyBlob.h:177, Doxygen, Priority: Normal

argument 'MyMessage' of command @param is not found in the argument list of MyBlob::getNext(MyMessage &m)

MyBlob.h:181, Doxygen, Priority: Normal

The following parameter of MyBlob::getNext(MyMessage &m) is not documented: parameter 'm'

cpetra · 2022-02-07T17:16:43Z

You're probably right.

Here is the new feedback from the build system:

Butler report

...
The following parameter of MyBlob::getNext(MyMessage &m) is not documented: parameter 'm'

Thank you for your patience! I have fixed the doxygen comments now.

virtual-maker · 2022-02-07T19:49:35Z

core/MyBlob.h

+
+#ifndef MYBLOB_H
+#define MYBLOB_H
+#pragma once


Remove #pragma once when you use:

#ifndef MYBLOB_H #define MYBLOB_H

Good point :)

Yes, but the #pragma once line is still there. I think you can simply remove this line.

And I was sure I did remove it.

cpetra · 2022-02-08T05:33:03Z

core/MyTransport.cpp

@@ -825,7 +826,15 @@ void transportProcessMessage(void)
 #endif //defined(MY_OTA_LOG_RECEIVER_FEATURE)
 #if defined(MY_GATEWAY_FEATURE)
 		// Hand over message to controller
-		(void)gatewayTransportSend(_msg);
+		if (_msg.getType() == V_MULTI_MESSAGE) {


Precious info! I have updated it now.

virtual-maker · 2022-02-07T21:15:45Z

keywords.txt

@@ -2,6 +2,7 @@
 # Datatypes (KEYWORD1)
 #######################################
 MyMessage	KEYWORD1
+MyBlob		KEYWORD1


Between MyBlob and KEYWORD1 must be a single tab char but not two. Otherwise, highlighting of this keyword in the Arduino IDE will not work.

My Arduino Studio highlights the keword fine though, but I have updated it.

cpetra · 2022-02-08T05:32:33Z

core/MyBlob.cpp

+ * repeater and gateway builds a routing tables in EEPROM which keeps track of the
+ * network topology allowing messages to be routed to nodes.
+ *
+ * Created by Constnatin Petra <constantin.petra@gmail.com>


Not that I care too much about the copyright, but I was under the impression that it's the file creation that the "Created by" line was about, and not the package. I saw the same approach on "core/MyGatewayTransport.cpp". Anyway, thanks for pointing out that I have mispelled my own name :) I have updated the copyright part now.

cpetra · 2022-02-08T05:32:43Z

core/workspace.code-workspace

@@ -0,0 +1,7 @@
+{


Yes, it got in by mistake, probably when doing the force push...

virtual-maker

All issues approved, but additional change requests will follow...

virtual-maker · 2022-02-08T22:35:13Z

core/MyTransport.cpp

+				MyMessage msg2;
+				MyBlob blob(&_msg);
+				while (blob.getNext(msg2)) {
+					receive(_msg);


This is a bug!
Line 845 should read:
receive(msg2);

With this change I was able to use this new MultiMessage N2N feature. Perfect!

OMG, can't quite get into the mood these days and have little time available, I apologize for this.

No problem, and now it is resolved. Thank you!

virtual-maker · 2022-02-08T23:01:28Z

@mfalkvidd
For my understanding from Mr. Jenkins output the Toll gate fails because of a warning in the new PJON stuff, e.g. in line PJONDefines.h:207:8. May be the included PJON library does not follow the MySensors coding style guidelines?

BTW I personally dislike the idea to include this big external PJON library into the MySensors lib repo. May be an other solution by use of an external library reference would be better. But I have no idea how to do this :-(

mfalkvidd · 2022-02-08T23:28:32Z

@mfalkvidd For my understanding from Mr. Jenkins output the Toll gate fails because of a warning in the new PJON stuff, e.g. in line PJONDefines.h:207:8. May be the included PJON library does not follow the MySensors coding style guidelines?

BTW I personally dislike the idea to include this big external PJON library into the MySensors lib repo. May be an other solution by use of an external library reference would be better. But I have no idea how to do this :-(

The Arduino IDE does not handle dependencies (at least not in an useful way). This has caused a lot of problems for MySensors users. The least bad way we’ve found so far is to include all code in the same repo. But that also means that we in effect fork every dependency, which sucks.

I’m not sure how we should handle the pjon case. I’ll think about it and see if there is something I think could work better. Ideas and suggestions are very welcome.

virtual-maker · 2022-02-09T21:18:15Z

The Arduino IDE does not handle dependencies (at least not in an useful way). This has caused a lot of problems for MySensors users. The least bad way we’ve found so far is to include all code in the same repo. But that also means that we in effect fork every dependency, which sucks.

I’m not sure how we should handle the pjon case. I’ll think about it and see if there is something I think could work better. Ideas and suggestions are very welcome.

I see, the issue is difficulty. I think it deserves a separate issue. I will create one...

virtual-maker · 2022-02-09T21:42:59Z

@cpetra and @mfalkvidd
OK, for this PR I still see following open tasks:

1. Renaming of MyBlob to MyMultiMessage,
2. Create/update a test for both use cases: N2N and N2G,
3. Create an associated issue as a feature request (1515 is not an issue but only this PR),
4. Changing the PR commit message by replacing this new FR Id, and then
5. A solid pledge that this new feature will be pulled into MySensors repo soon.

I will care for points 2 and 3. Do you agree with this? What about the other points?

cpetra · 2022-02-10T06:34:12Z

@cpetra and @mfalkvidd OK, for this PR I still see following open tasks:

1. Renaming of MyBlob to MyMultiMessage,

4. Changing the PR commit message by replacing this new FR Id, and then

I will care for points 2 and 3. Do you agree with this? What about the other points?
I can take care of 1 (and 4). For 1, it's already 2 against 1 and MyBlob naming is rather sentimental - it slowly got pushed towards "multi message" renaming anyway. Unfortunately I don't have time for this until tomorrow (4 also needs the "3" ID).

virtual-maker · 2022-02-10T21:28:02Z

@cpetra

I think this must be " > MAX_BLOB_SIZE", it's probably one byte lost.

I assume you refer to this location:
https://github.com/cpetra/MySensors/blob/e1ddbb4bd9e94617cb8ce2fa5fc1c72f7e69515d/core/MyBlob.cpp#L80

And yes, I agree this should read: if (_offset + size > MAX_BLOB_SIZE) {

virtual-maker · 2022-02-10T22:15:04Z

@cpetra
I have created associated issue #1517 - Add MultiMessage feature to combine and send multiple messages at once.

cpetra · 2022-02-11T18:18:23Z

Renamed MyBlob to MyMultiMessage; Also fixed line 80 in MyMultiMessage.cpp (that was the second update).

cpetra · 2022-02-20T18:09:30Z

@mfalkvidd @virtual-maker
Thanks for your implication! It was a very good experience for me!

@cpetra

Excellent contribution by @cpetra ! Thanks to @virtual-maker for extensive feedback on the PR.

mfalkvidd reviewed Feb 6, 2022

View reviewed changes

core/MyMessage.h Outdated Show resolved Hide resolved

mfalkvidd reviewed Feb 7, 2022

View reviewed changes

cpetra added a commit to cpetra/MySensors that referenced this pull request Feb 7, 2022

Add multi-message support (mysensors#1515)

e9f287d

cpetra force-pushed the development branch from 7c070d2 to e9f287d Compare February 7, 2022 05:25

cpetra added a commit to cpetra/MySensors that referenced this pull request Feb 7, 2022

Add multi-message support (mysensors#1515)

cdb8b65

cpetra force-pushed the development branch from e9f287d to cdb8b65 Compare February 7, 2022 17:12

cpetra changed the title ~~Added MyBlob feature, for fitting multiple sensor data in one message~~ Add multi-message feature, for handling multiple sensor data in one message Feb 7, 2022

virtual-maker requested changes Feb 7, 2022

View reviewed changes

cpetra added a commit to cpetra/MySensors that referenced this pull request Feb 8, 2022

Add multi-message support (mysensors#1515)

37506e6

cpetra force-pushed the development branch from cdb8b65 to 37506e6 Compare February 8, 2022 05:30

cpetra commented Feb 8, 2022

View reviewed changes

core/workspace.code-workspace Outdated

@@ -0,0 +1,7 @@

{

Copy link

Contributor Author

cpetra Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it got in by mistake, probably when doing the force push...

virtual-maker approved these changes Feb 8, 2022

View reviewed changes

virtual-maker reviewed Feb 8, 2022

View reviewed changes

cpetra added a commit to cpetra/MySensors that referenced this pull request Feb 9, 2022

Add multi-message support (mysensors#1515)

e1ddbb4

cpetra force-pushed the development branch from 37506e6 to e1ddbb4 Compare February 9, 2022 05:58

virtual-maker mentioned this pull request Feb 9, 2022

Need a better solution not to include PJON lib into MySensors repo #1516

Open

cpetra force-pushed the development branch from e1ddbb4 to b3ac96c Compare February 11, 2022 17:41

Add multi-message support (mysensors#1517)

57b21a0

cpetra force-pushed the development branch from b3ac96c to 57b21a0 Compare February 11, 2022 18:16

mfalkvidd merged commit d62ca6d into mysensors:development Feb 17, 2022

mfalkvidd added the release-notes Issues that have information that should be included in the release notes label Feb 17, 2022

virtual-maker mentioned this pull request Jun 25, 2022

Add CAN transport layer #1488

Open

tekka007 pushed a commit to tekka007/MySensors that referenced this pull request Jul 25, 2022

Add multi-message support (mysensors#1517) (mysensors#1515)

998079c

Excellent contribution by @cpetra ! Thanks to @virtual-maker for extensive feedback on the PR.

Uh oh!

Add multi-message feature, for handling multiple sensor data in one message #1515

Add multi-message feature, for handling multiple sensor data in one message #1515

Uh oh!

Conversation

cpetra commented Feb 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mfalkvidd commented Feb 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mfalkvidd commented Feb 6, 2022

Uh oh!

Uh oh!

virtual-maker commented Feb 6, 2022

Uh oh!

cpetra commented Feb 6, 2022

Uh oh!

virtual-maker commented Feb 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpetra commented Feb 7, 2022

Uh oh!

mfalkvidd commented Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpetra commented Feb 7, 2022

Uh oh!

mfalkvidd commented Feb 7, 2022

Uh oh!

cpetra commented Feb 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

virtual-maker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

virtual-maker commented Feb 8, 2022

Uh oh!

mfalkvidd commented Feb 8, 2022

Uh oh!

virtual-maker commented Feb 9, 2022

Uh oh!

virtual-maker commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpetra commented Feb 10, 2022

Uh oh!

virtual-maker commented Feb 10, 2022

Uh oh!

virtual-maker commented Feb 10, 2022

cpetra commented Feb 6, 2022 •

edited

Loading

mfalkvidd commented Feb 6, 2022 •

edited

Loading

mfalkvidd commented Feb 7, 2022 •

edited

Loading

virtual-maker commented Feb 9, 2022 •

edited

Loading