SNS - GPIO by Knhwong · Pull Request #34 · Hyp-ed/hyped-2023

Knhwong · 2022-11-04T23:30:46Z

All primary functionality done, I'll need to ask more about logging since using the pointers makes it harder to find problems.

ishmis · 2022-11-05T09:15:49Z

Please git merge master and resolve conflicts

ishmis

also, the scripts folder should not be touched at all

ishmis · 2022-11-05T09:17:35Z

+//GPIO  Start Addr   End Addr
+//GPIO0 0x44E0_7000  0x44E0_7FFF
+//GPIO1 0x4804_C000  0x4804_CFFF
+//GPIO2 0x481A_C000  0x481A_CFFF
+//GPIO3 0x481A_E000  0x481A_EFFF
+
+//GPIO_DATAIN (READ) 0x138h
+//GPIO_DATAOUT 0x13c
+//SET 0x194
+//CLEAR 0x190
+//  const uint8_t bank   = pin_ / 32;  // offset: GPIO_0,1,2,3
+//  const uint8_t pin_id = pin_ % 32;
+


commented code?

Referenced information for getting pin addresses. Can delete but anybody later on reading might find it useful.

If it's stuff you need to actually use in the code itself, don't keep it commented and just comment on the method that needs it desribing briefly how to use it.

ishmis · 2022-11-05T09:27:54Z

+  std::shared_ptr<HardwareGpioReader> reader;
+  return reader;


just returning an uninitialized reader?

ishmis · 2022-11-05T09:28:35Z

+  std::shared_ptr<HardwareGpioWriter> writer;
+  return writer;


uninitialized writer

maxguy2001 · 2022-11-06T12:28:51Z

+#include <sys/mman.h>
+#include <unistd.h>
+#include <fcntl.h>
+


maxguy2001 · 2022-11-06T12:29:12Z

-  // TODO: implement
-  throw -1;
+
+  int out = *gpio_readAddr & pinMAP;


const and style guide (naming)

maxguy2001 · 2022-11-06T12:29:32Z

 GpioWriteResult HardwareGpioWriter::write(const core::DigitalSignal state)
 {
-  // TODO: implement
+  //Not sure if this correct


Multiline comments?

maxguy2001 · 2022-11-06T12:29:43Z

+  //May just erase everything
+  //May need to 
+  if (state == core::DigitalSignal::kHigh) {
+    *gpio_setAddr |= pinMAP;


maxguy2001 · 2022-11-06T12:29:54Z

 }

+
+


maxguy2001 · 2022-11-06T12:34:02Z

+
+  const off_t pinAddress = bankAddresses[bank];
+  volatile void *gpio_addr;
+  volatile unsigned int *gpio_set;


maxguy2001 · 2022-11-06T12:36:18Z


+
+
+


maxguy2001 · 2022-11-06T12:36:44Z

 */
+
+//GPIO  Start Addr   End Addr
+//GPIO0 0x44E0_7000  0x44E0_7FFF


multiline comment...

maxguy2001 · 2022-11-06T12:37:19Z


 private:
  hyped::core::ILogger &log_;
+  const off_t bankAddresses[4] = {0x44e07000, 0x4804c000, 0x481ac000, 0x481ae000};


Where are these values from? Is it documented somewhere?

Taken from old code and also cross referenced from AM335x technical reference manual along with other tutorials.

Ok sounds good. Is this documented somewhere so that someone next year can know where these values came from?

Also in general "it was in the old codebase" is not justification. We are writing a new system here with lots of new components and there was lots of flawed legacy code in the 2022 codebase. If you are ever unsure of a value, please either ask someone who may already know (i.e. @SnickeyX) or find the relevant datasheet and document where the values came from.

maxguy2001 · 2022-11-06T12:37:39Z

+  std::unordered_map<uint8_t, std::shared_ptr<IGpioWriter>> InitializedWriters;
+  std::unordered_map<uint8_t, std::shared_ptr<IGpioReader>> InitializedReaders;
+
+  static constexpr unsigned int pinSize     = 0x1000;


uint and naming

ishmis · 2022-12-26T13:26:19Z

This PR definitely needs to be prioritized, @Knhwong don't think you updated the code here with the testing code that worked?

Knhwong · 2022-12-27T04:21:39Z

This PR definitely needs to be prioritized, @Knhwong don't think you updated the code here with the testing code that worked?

I sent you the testing code in a slack message, it was a basic main.cpp file that was a condensed gpio. The changes aka just type fixes were already updated in the main code.

ishmis · 2022-12-28T10:22:51Z

This PR definitely needs to be prioritized, @Knhwong don't think you updated the code here with the testing code that worked?

I sent you the testing code in a slack message, it was a basic main.cpp file that was a condensed gpio. The changes aka just type fixes were already updated in the main code.

are you not planning on updating it here?

Knhwong · 2022-12-29T01:22:35Z

This PR definitely needs to be prioritized, @Knhwong don't think you updated the code here with the testing code that worked?

I sent you the testing code in a slack message, it was a basic main.cpp file that was a condensed gpio. The changes aka just type fixes were already updated in the main code.

are you not planning on updating it here?

By "main" I am referring to this branch. In the main.cpp testing code, it's the same lines updated already within the getwriter or getreader on hardware_gpio.cpp back in the added comments and fixed_types commit. Or are you referring to something else here?

TomLonergan03

scripts/ doesn't contain anything

TomLonergan03 · 2023-01-30T23:15:06Z

+  const std::uint8_t out = *gpio_readAddr & pinMAP ? 1 : 0;
+
+  if (out > 0) {
+    return core::DigitalSignal::kHigh;
+  } else {
+    return core::DigitalSignal::kLow;
+  }


if (*gpio_readAddr & pinMAP)
also this method doesn't need to be optional

TomLonergan03 · 2023-01-30T23:16:19Z

+  } else {
+    *gpio_clearAddr = pinMAP;
+  }
+  return core::Result::kSuccess;


this can't return kFailure so no reason for returning a result

TomLonergan03 · 2023-01-30T23:17:23Z

+  // Integer divison to get bank number
+  const std::uint32_t bank = pin / 32;
+  // Modulo by 32 gets us the ID of the pin relative to the bank
+  const std::uint32_t pinID = pin % 32;


TomLonergan03 · 2023-01-30T23:17:47Z

+  // Modulo by 32 gets us the ID of the pin relative to the bank
+  const std::uint32_t pinID = pin % 32;
+  // Gpio addresses contain 32 pins, so we use pinmap to specify specific pin.
+  const std::uint32_t pinMAP = (1 << pinID);


pin_map unless MAP is an acronym then expand it

TomLonergan03 · 2023-01-30T23:18:14Z

+  }
+
+  // Get memory address from bank address
+  const off_t pinAddress = bankAddresses[bank];


pin_address

TomLonergan03 · 2023-01-30T23:22:00Z

+  const std::uint32_t pinID  = pin % 32;
+  const std::uint32_t pinMAP = (1 << pinID);


same naming issues as in getReader

TomLonergan03 · 2023-01-30T23:23:28Z

+  // Bank Addresses are header base addresses.
+  // Page 211-213 Figure 6-7/8 P8 Header Pins Beaglebone Bible
+
+  const off_t bankAddresses[4] = {0x44e07000, 0x4804c000, 0x481ac000, 0x481ae000};


bank_addresses, also should use c++ style array

TomLonergan03 · 2023-01-30T23:24:08Z

+
+  const off_t bankAddresses[4] = {0x44e07000, 0x4804c000, 0x481ac000, 0x481ae000};
+  std::unordered_map<std::uint8_t, std::shared_ptr<IGpioWriter>> InitializedWriters;
+  std::unordered_map<std::uint8_t, std::shared_ptr<IGpioReader>> InitializedReaders;


initialised_writers_ and initialised_readers_

TomLonergan03 · 2023-01-30T23:25:06Z

+static constexpr std::uint32_t pinSize  = 0x1000;
+static constexpr std::uint32_t pinRead  = 0x138;
+static constexpr std::uint32_t pinClear = 0x190;
+static constexpr std::uint32_t pinSet   = 0x194;


kPinSize, kPinRead and so on

TomLonergan03 · 2023-01-30T23:26:27Z


 private:
-  HardwareGpioReader();
+  HardwareGpioReader(std::uint8_t pin, volatile std::uint32_t *read)


const for pin

TomLonergan03 · 2023-02-20T01:06:14Z

+static constexpr std::uint32_t pin_size  = 0x1000;
+static constexpr std::uint32_t pin_read  = 0x138;
+static constexpr std::uint32_t pin_clear = 0x190;
+static constexpr std::uint32_t pin_set   = 0x194;


these become private static constexprs following the style guide discussion

unless they need to be accessed outside of methods

TomLonergan03 · 2023-02-20T01:08:32Z

+
 namespace hyped::io {

 std::optional<core::DigitalSignal> HardwareGpioReader::read()


optional with no nullopt return, so can just have return type core::DigitalSignal

TomLonergan03 · 2023-02-20T01:09:46Z

also what are the prehook etc files, they seem to be empty

Initial

258b43c

Knhwong requested review from ishmis and kshxtij as code owners November 4, 2022 23:30

Fixed shared dereference by adding to map

b83ffff

ishmis changed the title ~~Initial Commitment~~ SNS - GPIO Nov 5, 2022

ishmis assigned ishmis and Knhwong and unassigned ishmis Nov 5, 2022

ishmis added the sensors Sensors Project label Nov 5, 2022

ishmis requested changes Nov 5, 2022

View reviewed changes

maxguy2001 requested changes Nov 6, 2022

View reviewed changes

Knhwong mentioned this pull request Nov 7, 2022

SNS - GPIO #12

Closed

root added 7 commits November 7, 2022 18:47

Fixed Writer+Reader intialization and throw with small changes

da7c525

Fixed shared pointer

4f5bab5

Added loggin

e997646

Added Comments and fixed types

591b446

Merge branch 'master' into ss-gpioNew

0df80f8

Fixed compiler error

cbd231e

Removed hyped

fe3cc1f

root added 4 commits December 29, 2022 08:48

Clang-changes

a4fb12d

clang changes

f473200

test

e61a462

Fixed clang

a63d7e4

root added 2 commits December 29, 2022 09:24

Fixed std

e1c0bb3

clang

9578ad5

root added 6 commits December 29, 2022 09:29

Added std to uint_32 and 64

f742e05

Added comments

659e22f

ClangFormatted

07869e0

CF

ad58a84

cf2

ed16431

MacChanges?

032ea62

TomLonergan03 requested changes Jan 30, 2023

View reviewed changes

merged master

ab70cca

ishmis requested review from TomLonergan03 and maxguy2001 February 6, 2023 18:20

root added 2 commits February 6, 2023 19:14

Style fixes for most done. Will discuss about some changes.

646a4e1

clang

b53e8c0

TomLonergan03 requested changes Feb 20, 2023

View reviewed changes

		const std::uint32_t pinID = pin % 32;
		const std::uint32_t pinMAP = (1 << pinID);


		namespace hyped::io {

		std::optional<core::DigitalSignal> HardwareGpioReader::read()

Uh oh!

Conversation

Knhwong commented Nov 4, 2022

Uh oh!

ishmis commented Nov 5, 2022

Uh oh!

ishmis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ishmis commented Dec 26, 2022

Uh oh!

Knhwong commented Dec 27, 2022

Uh oh!

ishmis commented Dec 28, 2022

Uh oh!

Knhwong commented Dec 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomLonergan03 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Knhwong commented Dec 29, 2022 •

edited

Loading