Fix to allow get_packet during server boot #541
Conversation
ghost
requested review from
| value_type = value_type.to_s.intern | ||
| item_hash.each do |item_name, item_value| | ||
| packet.write(item_name, item_value, value_type) | ||
| if item_hash |
There was a problem hiding this comment.
This is required due to item_hash having a default value of nil
|
Not sure why the Java build failed. Are any of those return statements actually necessary or was this failure just a fluke? |
spec/script/telemetry_spec.rb
Outdated
| packet = get_packet(id) | ||
| expect(packet.target_name).to eql "SYSTEM" | ||
| expect(packet.packet_name).to eql "META" | ||
| expect(packet.received_time).to be_within(0.1).of Time.now |
There was a problem hiding this comment.
Why, do I need a delay after inject?
There was a problem hiding this comment.
Requiring anything to be within 0.1 seconds of now is too tight of tolerance and will sometimes fail.
| value_type = value_type.to_s.intern | ||
| item_hash.each do |item_name, item_value| | ||
| packet.write(item_name, item_value, value_type) | ||
| if item_hash |
lib/cosmos/script/telemetry.rb
Outdated
| results.delete_at(4) | ||
| end | ||
| results | ||
| $cmd_tlm_server.get_packet_data(id, non_block) |
There was a problem hiding this comment.
Doing this is a breaking change, please undo. Need to add smarts at get_packet or remove from shared.
There was a problem hiding this comment.
It was super confusing that this magic was happening here when I was trying to debug it. It's deprecated. Can we just remove it altogether?
There was a problem hiding this comment.
get_packet_data is a nice api for systems that don't know the whole definition of everything (like python). It's only really deprecated from Ruby... so we can't remove it.
Time doesn't serialize to JSON which is the only reason a time object wasn't sent directly. Ideally the api should give a ruby time object.
Having the return value be different between the internal API and the script api is confusing now though...
Any breaking changes bump us to COSMOS 4.1 which I don't want to goto now. Need to just keep it around for now and maybe we can remove the Ruby script version in 4.1.
lib/cosmos/script/api_shared.rb
Outdated
| packet.received_time = received_time | ||
| packet.received_count = received_count | ||
| packet.received_time = Time.at(rx_sec, rx_usec).sys | ||
| packet.received_count = rx_count | ||
| end | ||
| packet | ||
| end |
There was a problem hiding this comment.
Should probably just remove this from shared and redefine in both places. Making the change to get_packet_data at the script level is breaking backwards compat. Alternatively. could check the number of fields returned by get_packet_data.
lib/cosmos/script/scripting.rb
Outdated
| end | ||
| packet | ||
| end | ||
|
|
2 similar comments
|
@ryanatball One final review? |
|
👍 |
This one was tricky. My background task was trying to call get_packet but the telemetry.rb get_packet_data method wasn't being called so that little Time.at code wasn't getting called. I removed that to make get_packet_data just call the $cmd_tlm_server version with no extras and put the Time.at code in the api_shared get_packet code.
Did the telemetry.rb code not get called because I'm part of the server as a background task?
closes #540