Add support for with-wip #234

hamishwillee · 2018-10-25T02:26:06Z

Recently we added a tag <wip> for messages and enum values that indicates the message is a work in progress.

This adds a flag --with--wip that you must specify in a build to include those messages/values in the generated library. If you don't specify it, the messages are stripped (as discussed by @amilcarlucas here).

The generator strips the wip messages/enums after validation, but before generation.

hamishwillee · 2018-10-25T02:26:41Z

@peterbarker @amilcarlucas FYI. Please review and approve if you OK with this.

hamishwillee · 2018-10-25T03:53:32Z

FYI, My test file is as below. I ran it with a few different combinations of wip on different enum values and messages to validate message removal.

<?xml version="1.0"?>
<mavlink>
  <version>3</version>

<enums>
    <enum name="MAV_ENUM_NAME_ONE">
      <entry name="MAV_ENUM_NAME_ONE"> 
        <description />
      </entry>

      <entry name="MAV_ENUM_NAME_TWO_VALUE_ONE"> 
        <description />
      </entry>


</enum>

      <!-- enum with same name as enum -->
    <enum name="MAV_ENUM_NAME_TWO">
      <entry name="MAV_ENUM_NAME_TWO_VALUE_ONE"> 
        <description />
      </entry>
      <entry name="MAV_ENUM_NAME_TWO_VALUE_TWO"> 
        <description />
      </entry>
      <entry name="MAV_ENUM_NAME_TWO_VALUE_THREE"> 
        <wip />
        <description />
      </entry>
      <entry name="MAV_ENUM_NAME_TWO_VALUE_FOUR"> 
        <wip />
        <description />
      </entry>
      <entry name="MAV_ENUM_NAME_TWO_VALUE_FIVE"> 
        <description />
      </entry>
      <entry name="MAV_ENUM_NAME_TWO_VALUE_SIX"> 
        <description />
      </entry>
      <entry name="MAV_ENUM_NAME_TWO_VALUE_SEVEN">
        <wip />
        <description />
      </entry>
    </enum>
</enums>

  <messages>
    <message id="0" name="TEST_MSG_ONE">
      <description>Test one msg</description>
      <field type="char" name="c">char</field>
      <field type="char[10]" name="s">string</field>
    </message>
    <message id="1" name="TEST_MSG_TWO">
      <wip />
      <description>Test 2 msg</description>
      <field type="char" name="c">char</field>
      <field type="char[10]" name="s">string</field>
    </message>
    <message id="2" name="TEST_MSG_THREE">
      <wip />
      <description>Test 3 msg</description>
      <field type="char" name="c">char</field>
      <field type="char[10]" name="s">string</field>
    </message>
    <message id="3" name="TEST_MSG_FOUR">
      <description>Test 4 msg</description>
      <field type="char" name="c">char</field>
      <field type="char[10]" name="s">string</field>
    </message>
  </messages>
</mavlink>

hamishwillee · 2018-10-25T04:15:05Z

@peterbarker Actually I need your help here. The code works locally but fails on CI with:

  File "/home/travis/build/ArduPilot/pymavlink/generator/mavgen.py", line 139, in mavgen
    if not opts.with_wip:
AttributeError: 'Opts' object has no attribute 'with_wip'

But the argument appears to be defined properly in code

parser.add_argument("--with-wip", action="store_true", dest="with_wip", default=mavgen.DEFAULT_WITH_WIP, help="Include WIP messages in generated library.")

Thoughts ....

amilcarlucas · 2018-10-25T06:41:08Z

generator/mavparse.py

@@ -144,10 +145,11 @@ def __init__(self, index, description=''):
        self.description = description

 class MAVEnumEntry(object):
-    def __init__(self, name, value, description='', end_marker=False, autovalue=False, origin_file='', origin_line=0):
+    def __init__(self, name, value, description='', wip=False, end_marker=False, autovalue=False, origin_file='', origin_line=0):


I think it makes more sense to add the wip=false at the end of the parameter list instead of after the description

Ah, but you haven't considered that name, value, description AND wip are properties of the XML file and hence sensibly grouped. The others are properties of this parser. IMO doesn't make sense to shove at the end.

OK, makes sense

amilcarlucas

Many thanks, I planned to do this, but you beat me to it :)

amilcarlucas · 2018-10-25T06:44:05Z

generator/mavparse.py

@@ -257,7 +261,9 @@ def start_element(name, attrs):
                if (value > self.enum[-1].highest_value):
                    self.enum[-1].highest_value = value
                # append the new entry
-                self.enum[-1].entry.append(MAVEnumEntry(attrs['name'], value, '', False, autovalue, self.filename, p.CurrentLineNumber))
+                self.enum[-1].entry.append(MAVEnumEntry(attrs['name'], value, '', False, False, autovalue, self.filename, p.CurrentLineNumber))


Shouldn't the False be self.wip ?

No. At this point I don't know if the value is a wip or not. I discover that later on. I am using False as the default though because most entries are not wip.

amilcarlucas · 2018-10-25T06:48:10Z

One is with-wip the other is with_wip . Notice the underscore change

hamishwillee · 2018-10-25T07:07:17Z

One is with-wip the other is with_wip . Notice the underscore change

@amilcarlucas Yes, but I believe that is correct. The ArgumentParser takes an argument (what you actually enter of --with-wip) but the value that is stored and you test against is the dest - in this case with_wip. Having them different should be fine, as long as I test against the dest value - which I have.
It works locally, and this seems to be how the other opts work.

Or do I misunderstand your point?

amilcarlucas · 2018-10-25T07:23:54Z

regarding with-wip... forget about it. No coffee yet :P

hamishwillee · 2018-10-26T06:39:14Z

@peterbarker Ready to merge. The test fail is nothing to do with me. Honest guv.

^
../../include_v2.0/common/../mavlink_get_info.h:11:63: note: in expansion of macro ‘MAVLINK_MESSAGE_INFO’
  static const mavlink_message_info_t mavlink_message_info[] = MAVLINK_MESSAGE_INFO;
                                                               ^
make: *** [testmav2.0_common] Error 1
The command "./test_generator.sh" exited with 2.

amilcarlucas · 2018-10-26T08:33:18Z

Once this goes in, we can minimize the diff between the Ardupilot/mavlink fork and the upstream mavlink/mavlink :)

The ardupilot team does not want to have wip stuff in their fork, and this PR effectively prunes out wip stuff by default :)

hamishwillee · 2018-10-31T05:37:07Z

generator/mavparse.py

+
+        if not self.with_wip:
+            # remove wip elements from parsing
+            wip_entry = xmldocument.findall('.//wip/..')


FYI This is a bit messy because you can't delete an object from itself. Here I find the wip parents, then delete each of them from their parent.

hamishwillee · 2018-10-31T05:49:05Z

generator/mavparse.py

        p = xml.parsers.expat.ParserCreate()
        p.StartElementHandler = start_element
        p.EndElementHandler = end_element
        p.CharacterDataHandler = char_data
-        p.ParseFile(f)
+        xmlstring = etree.tostring(xmldocument)


FYI, - I use lxml.etree to remove the elements, then convert XML to a string that is then passed to the existing expat parser.

hamishwillee · 2018-10-31T05:54:11Z

@peterbarker The wip elements were still being recorded in CRC tables etc even if the actual messages were deleted. I don't know exactly exact failure mechanism because the tests passed locally, but obviously there should be no traces in generated code of elements that are removed, because that is not efficient.

Rather than attempt to clean the XML representation before output generation I have updated this to strip all WIP elements before they are validated or parsed. The code to record wip information in the parser has not been removed in case we want to keep WIP elements but still update the output generated for them.

Tests now pass. I think ready for a sanity check.

hamishwillee · 2018-11-12T04:24:39Z

@amilcarlucas Can you take this to the ArduPilot dev meeting? I just want to understand "when" it might be looked at.

amilcarlucas

Can this be merged soonish??

tridge · 2018-11-13T00:38:18Z

I think it would be better to auto-generate warning("Work in progress message") on use of any WIP msgs in the C interface
same with python

tridge · 2018-11-13T00:41:51Z

a bit more explanation ...
adding --with-wip will make it more painful for people to test WIP msgs, as the tools won't support them. For example, pymavlink releases from pip, mavproxy, mavlogdump, mavexplorer, qgc, MP etc won't support them. So users would need to rebuild their favorite tools to make use of and test a wip msg. Making testing of new features harder isn't great.
Instead, if we gave a warning on the use of the msg, then anyone could use it, but it is likely that we'd catch any use of them in a stable release, which is what we're trying to avoid

hamishwillee · 2018-11-13T02:42:02Z

@tridge I wish I had talked this through before doing the work - your point makes sense. The code to get this into the parser is present so I'll just strip out my message stripper and add the warning in. Sometime soon.

Fix up bug in wip removal Add some test code to see if can debug problem Remove test code added in previous commit Add with_wip to Opts() test class Strip wip elements before parsing Strip out wip command line option Remove unneeded changes

hamishwillee · 2018-11-14T00:00:56Z

Closing. Superseded by #240

hamishwillee mentioned this pull request Oct 25, 2018

common: add orbit execution telemetry message mavlink/mavlink#1006

Merged

amilcarlucas approved these changes Oct 25, 2018

View reviewed changes

amilcarlucas reviewed Oct 25, 2018

View reviewed changes

hamishwillee force-pushed the add_wip branch from 89494db to 64eaf2a Compare October 26, 2018 05:02

hamishwillee commented Oct 31, 2018

View reviewed changes

amilcarlucas approved these changes Nov 12, 2018

View reviewed changes

auturgy added the DevCallTopic label Nov 12, 2018

Add support for with-wip

b1f1945

Fix up bug in wip removal Add some test code to see if can debug problem Remove test code added in previous commit Add with_wip to Opts() test class Strip wip elements before parsing Strip out wip command line option Remove unneeded changes

hamishwillee force-pushed the add_wip branch from d19d58d to b1f1945 Compare November 13, 2018 06:23

hamishwillee closed this Nov 14, 2018

hamishwillee deleted the add_wip branch November 21, 2018 03:53

CraigElder removed the DevCallTopic label Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for with-wip #234

Add support for with-wip #234

hamishwillee commented Oct 25, 2018

hamishwillee commented Oct 25, 2018

hamishwillee commented Oct 25, 2018

hamishwillee commented Oct 25, 2018

amilcarlucas Oct 25, 2018

hamishwillee Oct 25, 2018

amilcarlucas Oct 25, 2018

amilcarlucas left a comment

amilcarlucas Oct 25, 2018

hamishwillee Oct 25, 2018

amilcarlucas Oct 25, 2018

amilcarlucas commented Oct 25, 2018

hamishwillee commented Oct 25, 2018

amilcarlucas commented Oct 25, 2018

hamishwillee commented Oct 26, 2018

amilcarlucas commented Oct 26, 2018

hamishwillee Oct 31, 2018

hamishwillee Oct 31, 2018

hamishwillee commented Oct 31, 2018

hamishwillee commented Nov 12, 2018

amilcarlucas left a comment

tridge commented Nov 13, 2018

tridge commented Nov 13, 2018

hamishwillee commented Nov 13, 2018

hamishwillee commented Nov 14, 2018

Add support for with-wip #234

Add support for with-wip #234

Conversation

hamishwillee commented Oct 25, 2018

hamishwillee commented Oct 25, 2018

hamishwillee commented Oct 25, 2018

hamishwillee commented Oct 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amilcarlucas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amilcarlucas commented Oct 25, 2018

hamishwillee commented Oct 25, 2018

amilcarlucas commented Oct 25, 2018

hamishwillee commented Oct 26, 2018

amilcarlucas commented Oct 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamishwillee commented Oct 31, 2018

hamishwillee commented Nov 12, 2018

amilcarlucas left a comment

Choose a reason for hiding this comment

tridge commented Nov 13, 2018

tridge commented Nov 13, 2018

hamishwillee commented Nov 13, 2018

hamishwillee commented Nov 14, 2018