Skip to content

Conversation

@mahajanrevant
Copy link
Contributor

This is a working version of ros2sim that implements our communication protocols to interact with the original solo8

@coveralls
Copy link

coveralls commented Mar 1, 2021

Pull Request Test Coverage Report for Build 611747165

  • 349 of 375 (93.07%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+7.5%) to 93.085%

Changes Missing Coverage Covered Lines Changed/Added Lines %
ros2sim/parsers/test_sim_executor.py 39 40 97.5%
ros2sim/parsers/sim_executor.py 42 45 93.33%
ros2sim/parsers/init.py 90 94 95.74%
ros2sim/sims/arduino_mocker.py 99 117 84.62%
Totals Coverage Status
Change from base Build 541473747: 7.5%
Covered Lines: 350
Relevant Lines: 376

💛 - Coveralls

@mahajanrevant mahajanrevant linked an issue Mar 1, 2021 that may be closed by this pull request
@mahajanrevant mahajanrevant requested a review from goobta March 1, 2021 20:21
Copy link
Member

@goobta goobta left a comment

Choose a reason for hiding this comment

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

This was a big one, great work! Just a handful of comments and questions.

Comment on lines +107 to +110
def get_joint_values(self, ):
"""Return a dictionary containing joint values based on joint ordering
"""
pass
Copy link
Member

Choose a reason for hiding this comment

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

Left over?

## This is important to preserve the exact values of joint values.
# Converting frop degrees to radians and back to degrees can cause some floating point
# differences to exist. This essentially gets rid of them.
values = np.round(values)
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't rounding cause a more significant error (floating point errors might exist, but our sensors are def not reading at that resolution)?

Comment on lines +95 to +96
if len(action_deg) != len(self.env.joint_ordering):
raise ValueError
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to be suppressing this exception? In other words, should we remove the try/except so that the program just exists?

Not sure what the intended behavior is, so your call.

Comment on lines +107 to +109
except:
raise ValueError('The action needs to have the same number of joint as '
'the robot')
Copy link
Member

Choose a reason for hiding this comment

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

Are you just reraising the same exception? Then you would want to do something like the following:

try:
  raise ValueError('Error1')
except:
  raise

The raise keyword will essentially reraise the exception that was caught. Re-raising the original exception allows the context of the exception to be maintained.

For example, consider the following:

try:
  if err = 'error1':
    raise ValueError('value1')
  elif err = 'error2':
    raise ValueError('value2')
else:
  raise

That way, you know exactly which error is being raised, even if they are the same error.

for i, rad in enumerate(ground_truth_rad):
if rad > math.pi:
ground_truth_rad[i] -= (2 * math.pi)
#print(self.env_stub.step.call_args[0][0])
Copy link
Member

Choose a reason for hiding this comment

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

nit : leftover?

import serial
import threading
import copy
import logging
Copy link
Member

Choose a reason for hiding this comment

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

Should make this into an issue, but we should make this into a named logger for file handling and whatnot.

Actually the best bet would be to make this into an issue: I think I have a code example somewhere on how to set up the loggers properly.

# Initial state of the state machine
self.serial_read_state = SerialReadState.INIT
# List of all possible states for the state machine
self.list_of_states = [state for state in SerialReadState]
Copy link
Member

Choose a reason for hiding this comment

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

is there no function in the enum that does this? i.e. list(SerialReadState)? genuinely have no idea.

Comment on lines +81 to +87
# while True:
# request = b''
# while not request.endswith(s.EOM.value):
# request += os.read(self.master, 1)

# response = self.parser.parse(request)
# os.write(self.master, response + s.EOM.value)
Copy link
Member

Choose a reason for hiding this comment

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

left over?

Comment on lines +76 to +79
else:
logging.debug("Updating Joint Angles")
logging.debug("The received data is: {}".format(self.validated_packed_data))
self.sim_executor.action(self.validated_packed_data)
Copy link
Member

Choose a reason for hiding this comment

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

No response / heartbeat on the action?

Comment on lines +210 to +223














No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

lmfao nit on the spaces

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.

Change default return from OK

4 participants