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

EHL-2 i2c library #13

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open

EHL-2 i2c library #13

wants to merge 29 commits into from

Conversation

sajidanower23
Copy link
Contributor

Science module lib

Copy link
Member

@hjed hjed left a comment

Choose a reason for hiding this comment

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

Lots of style comments. General logic looks good, but hard to tell without testing.

Lots of overlap between this and #12, I haven't repeated comments from #12

gnome-terminal --tab --command="bash -c '$openocd; $SHELL'" \
--tab --command="bash -c '$serial; $SHELL'" \
--tab --command="bash -c '$debug; $SHELL'" \
--disable-factory
Copy link
Member

Choose a reason for hiding this comment

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

would be better to use something more portable like tmux

@@ -0,0 +1,12 @@
Initialisation:
Copy link
Member

Choose a reason for hiding this comment

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

for all files that are in common please address comments on #12 before merging this (all the adc stuff)

@@ -0,0 +1,144 @@
#include "i2c.h"
Copy link
Member

Choose a reason for hiding this comment

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

header comment

SysCtlPeripheralReset(sysctl_module_i2c[module]);
}

//GPIOIntRegister(i2c_module[module], i2cIntHandler) // might do interrupts later
Copy link
Member

Choose a reason for hiding this comment

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

commented code

}

/*
void i2c_write(i2cModule_t module, uint8_t msg[], size_t length) {
Copy link
Member

Choose a reason for hiding this comment

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

commented code

PUBLIC
${science-mod_headers}
)
endfunction()
Copy link
Member

Choose a reason for hiding this comment

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

this is not the right place for this. Its not related to ros-echronos, probably belongs in the science-mod or /build-tools folder

include(${BUILD_TOOLS_DIR}/module_template.cmake)

#set(CPP_FILES adc_test.cpp can_wait_task.cpp)
set(CPP_FILES adc_test.cpp)
Copy link
Member

Choose a reason for hiding this comment

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

please include can_wait_task


/* Should never reach here, but if we do, an infinite loop is
easier to debug than returning somewhere random. */
for (;;) ;
Copy link
Member

Choose a reason for hiding this comment

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

see comments on #12

i2c_select(I2C0, module_num); // select multiplexer output
// UARTprintf("Initialising science servo to neutral position\n");
// servo_init(SCIENCE_SERVO, SCIENCE_SERVO_PIN);
// servo_write(SCIENCE_SERVO, SCIENCE_SERVO_PIN, NEUTRAL_POS);
Copy link
Member

Choose a reason for hiding this comment

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

commented code

// Create the publisher
ros_echronos::ROS_INFO("Data pub init\n");
owr_messages::science science_buffer_out[5];
ros_echronos::Publisher<owr_messages::science> science_pub("science/data", science_buffer_out, 5, false);
Copy link
Member

Choose a reason for hiding this comment

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

these should all have topics starting with / I'd actually try and match these with the voltmeter one in terms of naming as well

@hjed
Copy link
Member

hjed commented Sep 13, 2018

Also this repo is BSB + AGPL license

@hjed
Copy link
Member

hjed commented Sep 21, 2019

@burrrrrr what's happening with this pr?

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.

4 participants