Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modular decom #134

Merged
merged 5 commits into from
Sep 16, 2022
Merged

Modular decom #134

merged 5 commits into from
Sep 16, 2022

Conversation

ryanmelt
Copy link
Member

@ryanmelt ryanmelt commented Sep 9, 2022

closes #27

@ryanmelt ryanmelt requested a review from jmthomas September 9, 2022 16:56
@codecov
Copy link

codecov bot commented Sep 9, 2022

Codecov Report

Merging #134 (ca10ae2) into master (2fe19e2) will increase coverage by 0.16%.
The diff coverage is 97.19%.

@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
+ Coverage   73.66%   73.83%   +0.16%     
==========================================
  Files         426      433       +7     
  Lines       25558    25809     +251     
  Branches      559      559              
==========================================
+ Hits        18827    19055     +228     
- Misses       6647     6670      +23     
  Partials       84       84              
Flag Coverage Δ
ruby-api 46.89% <ø> (ø)
ruby-backend 78.17% <97.16%> (+0.20%) ⬆️

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

Impacted Files Coverage Δ
openc3/lib/openc3/packets/packet_config.rb 97.75% <77.77%> (-1.24%) ⬇️
openc3/lib/openc3/accessors/accessor.rb 88.00% <88.00%> (ø)
openc3/lib/openc3/accessors/json_accessor.rb 90.16% <90.16%> (ø)
openc3/lib/openc3/accessors/cbor_accessor.rb 90.90% <90.90%> (ø)
openc3/lib/openc3/accessors/xml_accessor.rb 96.96% <96.96%> (ø)
openc3/lib/openc3/packets/packet.rb 98.20% <98.14%> (-0.03%) ⬇️
openc3/lib/openc3/accessors/binary_accessor.rb 99.06% <99.06%> (ø)
...-cmdtlmserver/src/tools/CmdTlmServer/RawDialog.vue 89.47% <100.00%> (ø)
openc3/lib/openc3.rb 95.45% <100.00%> (+0.21%) ⬆️
openc3/lib/openc3/accessors.rb 100.00% <100.00%> (ø)
... and 13 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@jmthomas jmthomas left a comment

Choose a reason for hiding this comment

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

I need an example in the demo of how this works on the user side. Don't need every accessor but at least JSON. Also you probably need more unit tests.

@@ -1032,6 +1112,22 @@ def self.from_json(hash)
packet
end

def decom
# Read all the RAW at once because this could be optimized by the accessor
json_hash = read_items(@sorted_items)
Copy link
Member

Choose a reason for hiding this comment

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

So read_items initializes the hash and then we call read_item again but pass given_raw to really populate it? I'm confused about the 2 pass system here.

endianness, hash['array_size'], overflow)
si.key = hash['key'] || hash['name']
Copy link
Member

@jmthomas jmthomas Sep 10, 2022

Choose a reason for hiding this comment

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

You didn't want to add key to the constructor? :-)

IGNORE_PARAMETER ID_ITEM

CMD_UNIQUE_ID_MODE
TLM_UNIQUE_ID_MODE
Copy link
Member

Choose a reason for hiding this comment

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

Are these required for the new accessor modes? Or if you have the exact same ID_ITEM KEY then no?

Copy link
Member Author

Choose a reason for hiding this comment

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

They are not required if all the packets have the same Accessor, ID_ITEMS, and keys

# Create the item
item = @item_class.new(name_upcase, bit_offset, bit_size, data_type, endianness, array_size, overflow)
item = @item_class.new(name, bit_offset, bit_size, data_type, endianness, array_size, overflow)
Copy link
Member

Choose a reason for hiding this comment

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

Does this have other repercussions?

Copy link
Member Author

Choose a reason for hiding this comment

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

It gets upcased again farther down

@jmthomas
Copy link
Member

I love the new example project with the example of all the different ways you can access the data. This will make it much easier to use for end users. Nicely done!

@ryanmelt
Copy link
Member Author

Tests pass locally. Merging for now.

@ryanmelt ryanmelt merged commit a9ab663 into master Sep 16, 2022
@ryanmelt ryanmelt deleted the modular_decom branch September 16, 2022 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modular Decommutation Logic
2 participants