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

nos3#104 - Generic EPS Component #1

Merged
merged 8 commits into from
Mar 1, 2023
Merged

nos3#104 - Generic EPS Component #1

merged 8 commits into from
Mar 1, 2023

Conversation

jlucas9
Copy link
Member

@jlucas9 jlucas9 commented Feb 9, 2023

No description provided.

@jlucas9 jlucas9 requested review from mgrubb-gsfc and msuder February 9, 2023 14:15
@jlucas9 jlucas9 self-assigned this Feb 9, 2023
@jlucas9 jlucas9 mentioned this pull request Feb 9, 2023
3 tasks
Copy link
Contributor

@mgrubb-gsfc mgrubb-gsfc left a comment

Choose a reason for hiding this comment

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

Looks good. Tested

README.md Outdated
This includes flight software (FSW), ground software (GSW), simulation, and support directories.

## Overview
This generic_eps component is a UART device that accepts multiple commands, including requests for telemetry and data.
This generic eps component is an I2C device that accepts commands via writes to specific addresses in memory and returns status in every response.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to mention memory? how about "via data sent from the I2C master"?
Maybe change "in every response" to "in response to every command"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in commit 261a489

- uint16, temperature
* Solar Array
- uint16, voltage
- uint16, temperature
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to batteries, I guess only one solar array connection to the EPS or this is summary information for all connections?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in commit 261a489

README.md Outdated
<current>0.25</current>
<hex-status>0000</hex-status>
</switch-0>
...
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would recommend filling in the entire block so that a user can just copy the block from here and paste it into the nos3-simulator.xml block. In addition, you might look at the latest generic_fss/sim component as I decided to actually add file snippets in a cfg subdirectory (more easily "snippable"... maybe we will automate "building" the nos3-simulator.xml file in the future) and then reference them in the README (DRY principle).

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in commit 261a489


#include <sim_i_data_provider.hpp>
#include <generic_eps_data_point.hpp>
#include <sim_i_hardware_model.hpp>

#include <iostream>
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete, not needed.

sim_logger->info("Generic_epsHardwareModel::Generic_epsHardwareModel: Now on I2C bus name %s as address 0x%02x.", bus_name.c_str(), bus_address);

/* Get on the command bus - EPS receives commands here */
_time_bus_name = "command";
Copy link
Contributor

Choose a reason for hiding this comment

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

Change _time_bus_name to _command_bus_name and declare it as std::string here (removing it from the class definition... it is never used outside this constructor).

/* Found it... don't need to go through any more items*/
break;
}
}
}
_time_bus.reset(new NosEngine::Client::Bus(_hub, connection_string, time_bus_name));
sim_logger->info("Generic_epsHardwareModel::Generic_epsHardwareModel: Now on time bus named %s.", time_bus_name.c_str());
_time_bus.reset(new NosEngine::Client::Bus(_hub, connection_string, _time_bus_name));
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete this and the next line and _time_bus... they aren't used.

/* Is the status valid? */
if ((sw_status == 0x00) || (sw_status == 0xAA))
{
/* Prepare the simulator bus with the switch node */
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete this and the next line... dead, useless code

Copy link
Member Author

Choose a reason for hiding this comment

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

I think printing in the log that an invalid state write occurred is useful

@@ -16,9 +16,8 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently this code is untestable with real hardware because there is none.
This code is untestable with the simulator because of issues building with the simulator hwlib (but it is not required for testing the sim... the sim can be tested with fsw, the sim terminal, or by other means... so it is not high priority to fix this).
So currently this code seems to have no purpose... but it still seems valuable if one of the two previous statements is resolved, so we want to keep this code as a model for the future.
Agreed upon result: 1. Add some comments in the code to describe this situation. 2. Write an issue that it is broken with respect to building with the simulator hwlib.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wrote issue nasa/nos3#146. Once code comment is added, this comment can be resolved.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a reason to wait for other issues to be completed if they're reported already?

@jlucas9 jlucas9 changed the base branch from main to dev March 1, 2023 15:20
@jlucas9 jlucas9 merged commit f134ade into dev Mar 1, 2023
@jlucas9 jlucas9 deleted the nos3#104-EPS branch June 13, 2023 21:03
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.

3 participants